Skip to content

Commit

Permalink
fix: noir-compiler breadth-first resolver (#3307)
Browse files Browse the repository at this point in the history
This PR fixes an issue hit by @spypsy and @catmcgee last week while
working on a sample contract. They were not able to compile their
contract because the compiler failed to find all of the needed
dependencies.

I've identified the issue with `NoirDependencyManager` resolving
dependencies in a depth-first manner. This caused issues with libraries
that had dependencies of their own linked to by relative paths. This was
a problem because the new pipeline using wasm only unpacks the required
folder from a github archive (while Nargo gets the whole thing). In the
contract's case, it had dependencies on easy_private_state and
value_note (in this order) but easy_private_state needed value_note to
exist before the manager resolved it.

This PR replaces the algorithm with a breadth-first resolver which fixes
this. It also adds updates the existing unit test for the manager to
check this edge case.
  • Loading branch information
alexghr authored Nov 17, 2023
1 parent dd9dd84 commit 02348cf
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 22 deletions.
2 changes: 0 additions & 2 deletions yarn-project/noir-compiler/src/cli/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,11 @@ export function compileContract(program: Command, name = 'contract', log: LogFn
const tsPath = resolve(projectPath, typescript, `${contract.name}.ts`);
log(`Writing ${contract.name} typescript interface to ${path.relative(currentDir, tsPath)}`);
let relativeArtifactPath = path.relative(path.dirname(tsPath), artifactPath);
log(`Relative path: ${relativeArtifactPath}`);
if (relativeArtifactPath === `${contract.name}.json`) {
// relative path edge case, prepending ./ for local import - the above logic just does
// `${contract.name}.json`, which is not a valid import for a file in the same directory
relativeArtifactPath = `./${contract.name}.json`;
}
log(`Relative path after correction: ${relativeArtifactPath}`);
const tsWrapper = generateTypescriptContractInterface(contract, relativeArtifactPath);
mkdirpSync(path.dirname(tsPath));
writeFileSync(tsPath, tsWrapper);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ describe('DependencyManager', () => {
lib2: {
path: '/lib2',
},
lib3: {
path: '/lib3',
},
},
package: {
name: 'test_contract',
Expand All @@ -38,7 +41,7 @@ describe('DependencyManager', () => {

it('resolves root dependencies', async () => {
await manager.resolveDependencies();
expect(manager.getEntrypointDependencies()).toEqual(['lib1', 'lib2']);
expect(manager.getEntrypointDependencies()).toEqual(['lib1', 'lib2', 'lib3']);
});

it('resolves library dependencies', async () => {
Expand Down Expand Up @@ -75,7 +78,7 @@ class TestDependencyResolver implements NoirDependencyResolver {
package: new NoirPackage('/lib2', '/lib2/src', {
dependencies: {
lib3: {
path: '/lib3',
path: '../lib3',
},
},
package: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class NoirDependencyManager {
* Resolves dependencies for a package.
*/
public async resolveDependencies(): Promise<void> {
await this.#recursivelyResolveDependencies('', this.#entryPoint);
await this.#breadthFirstResolveDependencies();
}

/**
Expand All @@ -59,26 +59,46 @@ export class NoirDependencyManager {
return dep?.version;
}

async #recursivelyResolveDependencies(packageName: string, noirPackage: NoirPackage): Promise<void> {
for (const [name, config] of Object.entries(noirPackage.getDependencies())) {
// TODO what happens if more than one package has the same name but different versions?
if (this.#libraries.has(name)) {
this.#log(`skipping already resolved dependency ${name}`);
async #breadthFirstResolveDependencies(): Promise<void> {
/** Represents a package to resolve dependencies for */
type Job = {
/** Package name */
packageName: string;
/** The package location */
noirPackage: NoirPackage;
};

const queue: Job[] = [
{
packageName: '',
noirPackage: this.#entryPoint,
},
];

while (queue.length > 0) {
const { packageName, noirPackage } = queue.shift()!;
for (const [name, config] of Object.entries(noirPackage.getDependencies())) {
// TODO what happens if more than one package has the same name but different versions?
if (this.#libraries.has(name)) {
this.#log(`skipping already resolved dependency ${name}`);
this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]);

continue;
}
const dependency = await this.#resolveDependency(noirPackage, config);
if (dependency.package.getType() !== 'lib') {
this.#log(`Non-library package ${name}`, config);
throw new Error(`Dependency ${name} is not a library`);
}

this.#libraries.set(name, dependency);
this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]);

continue;
queue.push({
noirPackage: dependency.package,
packageName: name,
});
}

const dependency = await this.#resolveDependency(noirPackage, config);
if (dependency.package.getType() !== 'lib') {
this.#log(`Non-library package ${name}`, config);
throw new Error(`Dependency ${name} is not a library`);
}

this.#libraries.set(name, dependency);
this.#dependencies.set(packageName, [...(this.#dependencies.get(packageName) ?? []), name]);

await this.#recursivelyResolveDependencies(name, dependency.package);
}
}

Expand Down

0 comments on commit 02348cf

Please sign in to comment.