fix: sorting artifact props and members in metadata #9772

sklppy88 · 2024-11-06T09:56:05Z

After discussions with @spalladino, it was decided to use the schema parser to enforce object property ordering, and a schema transform to enforce array ordering. This PR is enabled to work by the schema work done by @spalladino, and simply sorts our known arrays that are normally outputted in a non-deterministic fashion by the noir compiler.

sklppy88 · 2024-11-06T09:56:20Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

spalladino

Good catch, thanks!! As for the implementation itself, maybe it makes more sense to just use the replacer in JSON stringify and reorder items there, as opposed to doing a structured clone, then sorting, and then stringifying? Also, tests!

sklppy88 · 2024-11-07T16:01:27Z

Good catch, thanks!! As for the implementation itself, maybe it makes more sense to just use the replacer in JSON stringify and reorder items there, as opposed to doing a structured clone, then sorting, and then stringifying? Also, tests!

Thanks for the 👀 ! Noted on the tests !

Regarding using the replacer, I had indeed thought about that, but the issue I ran into was with non-homogenous objects in arrays, which we need to deal with. i.e. if we are always sorting at the same level, how do we properly sort an array with objects with different sets of props that themselves may not be ordered, or be further nested objects / arrays ? I think this is doable, but introduces a litany of other problems, and I think ends up being more complex, hence why I opted for the bottom up approach w/ hashes. Or maybe I'm misunderstanding your comment completely and missed something when I was initially looking at this. Totally possible. 🙃

spalladino · 2024-11-07T16:19:09Z

Regarding using the replacer, I had indeed thought about that, but the issue I ran into was with non-homogenous objects in arrays, which we need to deal with. i.e. if we are always sorting at the same level, how do we properly sort an array with objects with different sets of props that themselves may not be ordered, or be further nested objects / arrays ? I think this is doable, but introduces a litany of other problems, and I think ends up being more complex, hence why I opted for the bottom up approach w/ hashes. Or maybe I'm misunderstanding your comment completely and missed something when I was initially looking at this. Totally possible. 🙃

Ah I see what you mean. You're right, trying to solve this for an unknown structure is difficult: I hadn't noticed that array ordering didn't matter in the contract artifact. What I'm worried about now is that array ordering should matter in some parts of the artifact (eg the order of inputs or outputs to a function), and with the current implementation, we're bypassing it.

I think the safest solution should be to tweak the loadContractArtifact function, which we use for adapting noir compilation output to our artifact format, so that it sorts its children as needed. And then we can safely stringify stuff, knowing it was sorted in advance when loaded.

nventuro · 2024-11-07T14:13:17Z

yarn-project/circuits.js/src/contract/artifact_hash.ts

-  // TODO: #6021 We need to make sure the artifact is deterministic from any specific compiler run. This relates to selectors not being sorted and being
-  // apparently random in the order they appear after compiled w/ nargo. We can try to sort this upon loading an artifact.
-  // TODO: #6021: Should we use the sorted event selectors instead? They'd need to be unique for that.


Should these issues not be closed?

nventuro · 2024-11-13T19:05:55Z

yarn-project/circuit-types/src/interfaces/pxe.test.ts

@@ -43,6 +44,8 @@ import { TxExecutionRequest } from '../tx_execution_request.js';
 import { type EventMetadataDefinition, type PXE, type PXEInfo, PXESchema } from './pxe.js';
 import { type SyncStatus } from './sync-status.js';

+jest.setTimeout(12_000);


Is this a leftover from debugging?

nventuro · 2024-11-13T19:07:26Z

yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts

@@ -40,6 +40,8 @@ describe('unconstrained_function_membership_proof', () => {
    expect(artifact.functions.filter(isUnconstrained).length).toBe(1);

    const unconstrainedFunction = unconstrainedFns[0];
+    const selector = FunctionSelector.fromNameAndParameters(unconstrainedFunction);


This shadows the selector variable in the test - I'd pick a different name, or pass this value directly to the fn call

(also why do we need to recompute this if we're already doing it in the beforeEach block?)

nventuro · 2024-11-13T19:12:36Z

yarn-project/foundation/src/abi/abi.ts

+        // We are manually ordering events and functions in the abi by path.
+        // The path ordering is arbitrary, and only needed to ensure deterministic order.


Suggested change

// We are manually ordering events and functions in the abi by path.

// The path ordering is arbitrary, and only needed to ensure deterministic order.

// We are manually ordering events and functions in the abi by path.

// The path ordering is arbitrary, and only needed to ensure deterministic order.

// These are the only arrays in the artifact with arbitrary order, and hence the only ones

// we need to sort.

sklppy88 changed the title ~~init~~ fix: sorting artifact props and members in metadata Nov 6, 2024

sklppy88 marked this pull request as ready for review November 6, 2024 09:57

sklppy88 mentioned this pull request Nov 6, 2024

fix: make bytecode part of artifact hash preimage again #9771

Open

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch 2 times, most recently from 30949fe to eb6053f Compare November 6, 2024 17:31

spalladino reviewed Nov 7, 2024

View reviewed changes

nventuro reviewed Nov 7, 2024

View reviewed changes

sklppy88 linked an issue Nov 11, 2024 that may be closed by this pull request

[SECURITY] Make bytecode part of artifact hash preimage again #5860

Open

sklppy88 removed a link to an issue Nov 11, 2024

[SECURITY] Make bytecode part of artifact hash preimage again #5860

Open

sklppy88 linked an issue Nov 11, 2024 that may be closed by this pull request

Sort props in noir contract artifact when loading #6021

Open

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from eb6053f to 8c28605 Compare November 12, 2024 18:14

sklppy88 changed the base branch from master to ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts November 12, 2024 18:14

This was referenced Nov 12, 2024

fix: add zod parsing for generated contract artifacts #9905

Open

chore: fixing test contract fixture #9909

Merged

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch from 130f512 to 5e6474b Compare November 12, 2024 21:16

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from 8c28605 to 42a5096 Compare November 12, 2024 21:16

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch from 5e6474b to 13c8846 Compare November 12, 2024 21:18

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from 42a5096 to d2cfd86 Compare November 12, 2024 21:20

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch from 13c8846 to fd68e26 Compare November 12, 2024 21:22

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from d2cfd86 to 96ed13b Compare November 12, 2024 21:25

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch 2 times, most recently from ac38581 to 65ad681 Compare November 13, 2024 15:40

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch 2 times, most recently from 92c7977 to 83f31ed Compare November 13, 2024 15:48

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch from 65ad681 to 6019397 Compare November 13, 2024 18:49

nventuro approved these changes Nov 13, 2024

View reviewed changes

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from 83f31ed to a48fc61 Compare November 13, 2024 19:23

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch 3 times, most recently from b8bfe75 to 0faa106 Compare November 13, 2024 20:06

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch 2 times, most recently from 4c8b87e to 59c4ead Compare November 13, 2024 20:48

sklppy88 added the e2e-all CI: Enables this CI job. label Nov 13, 2024

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch from 0faa106 to 814019b Compare November 14, 2024 13:49

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from 59c4ead to 0ae9662 Compare November 14, 2024 13:50

sklppy88 force-pushed the ek/fix/6021/add-zod-parsing-for-generated-contract-artifacts branch 2 times, most recently from 7b804af to 52abaf5 Compare November 14, 2024 21:06

use validation lib to enforce sorting and ordering

2dadc84

sklppy88 force-pushed the ek/fix/6021/sort-artifact-props-and-members-in-metadata branch from 0ae9662 to 2dadc84 Compare November 14, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sorting artifact props and members in metadata #9772

fix: sorting artifact props and members in metadata #9772

sklppy88 commented Nov 6, 2024 •

edited

Loading

sklppy88 commented Nov 6, 2024 •

edited

Loading

spalladino left a comment

sklppy88 commented Nov 7, 2024 •

edited

Loading

spalladino commented Nov 7, 2024

nventuro Nov 7, 2024

nventuro Nov 13, 2024

nventuro Nov 13, 2024

nventuro Nov 13, 2024

nventuro Nov 13, 2024

		// We are manually ordering events and functions in the abi by path.
		// The path ordering is arbitrary, and only needed to ensure deterministic order.

fix: sorting artifact props and members in metadata #9772

Are you sure you want to change the base?

fix: sorting artifact props and members in metadata #9772

Conversation

sklppy88 commented Nov 6, 2024 • edited Loading

sklppy88 commented Nov 6, 2024 • edited Loading

spalladino left a comment

Choose a reason for hiding this comment

sklppy88 commented Nov 7, 2024 • edited Loading

spalladino commented Nov 7, 2024

nventuro Nov 7, 2024

Choose a reason for hiding this comment

nventuro Nov 13, 2024

Choose a reason for hiding this comment

nventuro Nov 13, 2024

Choose a reason for hiding this comment

nventuro Nov 13, 2024

Choose a reason for hiding this comment

nventuro Nov 13, 2024

Choose a reason for hiding this comment

sklppy88 commented Nov 6, 2024 •

edited

Loading

sklppy88 commented Nov 6, 2024 •

edited

Loading

sklppy88 commented Nov 7, 2024 •

edited

Loading