Include new contract data function tree root in tx hash#188
Merged
Conversation
spalladino
force-pushed
the
palla/tx-hash-with-fn-tree-root
branch
4 times, most recently
from
4ae9246 to
65ea927
Compare
spalladino
force-pushed
the
palla/tx-hash-with-fn-tree-root
branch
from
ba5780a to
356689a
Compare
ludamad
reviewed
Apr 10, 2023
| import { BarretenbergWasm } from '../../wasm/index.js'; | ||
| import { WasmWrapper } from '@aztec/foundation/wasm'; | ||
|
|
||
| export class Grumpkin { |
Collaborator
There was a problem hiding this comment.
Not the biggest fan of the Wrapper name, the intent was that the generic concept was called WasmModule
Collaborator
There was a problem hiding this comment.
Almost any utility class could be called a Wrapper, is my reasoning, but it represents what it wraps
Contributor
Author
There was a problem hiding this comment.
Agree, and WasmModule was also my first choice for a name, but there's already a lower level WasmModule in the repo.
Collaborator
There was a problem hiding this comment.
Yeah I created that one, it was meant to provide everything needed to just pass around without introducing base classes
Collaborator
There was a problem hiding this comment.
I suppose that's not quite right though with async call state
ludamad
reviewed
Apr 10, 2023
| COPY l2-block l2-block | ||
| COPY tx tx | ||
| COPY unverified-data unverified-data | ||
| COPY . . |
Collaborator
There was a problem hiding this comment.
+1 if we're going to have a dependency system no reason to copy dependencies one by one (which mostly can help with docker figuring out its own dependencies)
ludamad
approved these changes
Apr 10, 2023
ludamad
left a comment
ludamad
left a comment
Collaborator
There was a problem hiding this comment.
LGTM other than naming comment
Contributor
Author
|
I cannot come up with a better name for now. I'll merge and keep thinking about it. |
PhilWindle
added a commit
that referenced
this pull request
Jun 8, 2026
#23921) Fixes A-836 (Audit #188). ## Problem The CLI loaded contract artifacts via `loadContractArtifact` (`getContractArtifact` in `cli/src/utils/aztec.ts`). For an **already-processed** artifact, `loadContractArtifact` only runs the shallow `isContractArtifact` heuristic — its own JSDoc notes "The check is not exhaustive". It checks `name`/`functions`/`nonDispatchPublicFunctions` shape but never validates parameter ABIs, types, storage layout, outputs, etc. (Raw nargo output is already fully validated, since it flows through `generateContractArtifact` → `ContractArtifactSchema.parse`.) A malformed-but-superficially-shaped artifact therefore bypassed schema validation and surfaced as an opaque error later during deployment/arg-encoding rather than a clear validation failure. ## Fix Add `loadContractArtifactWithValidation` in stdlib: it runs the full `ContractArtifactSchema` over an already-processed artifact before returning, and otherwise defers to `loadContractArtifact` (raw nargo stays validated as before). The returned object is identical to `loadContractArtifact`'s — the schema parse is used purely as a validation gate. The CLI's `getContractArtifact` now uses it. Scoped deliberately to the CLI (the audit's concern). `loadContractArtifact` is called at 100+ module-load sites with wire-form JSON; adding a stricter schema gate to all of them would have a large blast radius and risk rejecting legacy artifacts, so the unguarded function is left unchanged. ## Test Added `loadContractArtifactWithValidation` tests in `contract_artifact.test.ts`: a valid already-processed artifact (wire form) loads, and one whose `functionType` is a non-enum string — which still passes the shallow `isContractArtifact` check — is rejected. The rejection test fails without the schema gate and passes with it. ## Note Severity is MEDIUM in the finding, but as the artifact is local input the deployer supplies (not an untrusted network boundary) and downstream code still throws on bad input, the practical impact is closer to LOW (a clearer error message, earlier).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note that, since we now depend on circuits for explicitly calculating contract leaves via the
computeContractLeafcbind, the primitives wasm is left unused. I'm leaving it as part of the PR since we can leverage it in the future for more lightweight dependencies, but I wouldn't worry about this atm.Depends on AztecProtocol/aztec3-circuits#178
Fixes #154