Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tiered DI #52

Merged
merged 12 commits into from
Oct 23, 2023
4 changes: 3 additions & 1 deletion framework.iml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
<module type="WEB_MODULE" version="4">
<component name="NewModuleRootManager" inherit-compiler-output="true">
<exclude-output />
<content url="file://$MODULE_DIR$" />
<content url="file://$MODULE_DIR$">
maht0rz marked this conversation as resolved.
Show resolved Hide resolved
<excludePattern pattern="dist" />
</content>
<orderEntry type="sourceFolder" forTests="false" />
</component>
</module>
5 changes: 5 additions & 0 deletions packages/common/src/config/ChildContainerProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { DependencyContainer } from "tsyringe";

export interface ChildContainerProvider {
(): DependencyContainer;
}
5 changes: 5 additions & 0 deletions packages/common/src/config/ChildContainerStartable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { ChildContainerProvider } from "./ChildContainerProvider";

export interface ChildContainerStartable {
maht0rz marked this conversation as resolved.
Show resolved Hide resolved
create: (childContainerProvider: ChildContainerProvider) => void;
}
14 changes: 13 additions & 1 deletion packages/common/src/config/ConfigurableModule.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
import { noop } from "../utils";

import { ChildContainerProvider } from "./ChildContainerProvider";
import { ChildContainerStartable } from "./ChildContainerStartable";

const errors = {
configNotSet: (moduleName: string) =>
new Error(
Expand All @@ -17,7 +22,9 @@ export interface Configurable<Config> {
/**
* Used by various module sub-types that may need to be configured
*/
export class ConfigurableModule<Config> implements Configurable<Config> {
export class ConfigurableModule<Config>
implements Configurable<Config>, ChildContainerStartable
{
/**
* Store the config separately, so that we can apply additional
* checks when retrieving it via the getter
Expand All @@ -36,6 +43,11 @@ export class ConfigurableModule<Config> implements Configurable<Config> {
public set config(config: Config) {
this.currentConfig = config;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
public create(childContainerProvider: ChildContainerProvider): void {
noop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling @typescript-eslint/no-empty-function for this line would be nicer than noop()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but then why do we have this rule when we don't want the thing that the rule forces us to do?

}
}

// Helps ensure that the target class implements static presets
Expand Down
74 changes: 67 additions & 7 deletions packages/common/src/config/ModuleContainer.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
/* eslint-disable max-lines */
import "reflect-metadata";

import { container, Frequency, InjectionToken, Lifecycle } from "tsyringe";
import {
DependencyContainer,
Frequency,
InjectionToken,
Lifecycle,
} from "tsyringe";
import log from "loglevel";

import { StringKeyOf, TypedClass } from "../types";
import { DependencyFactory } from "../dependencyFactory/DependencyFactory";

import { Configurable, ConfigurableModule } from "./ConfigurableModule";
import { ChildContainerProvider } from "./ChildContainerProvider";
import { ChildContainerStartable } from "./ChildContainerStartable";

const errors = {
configNotSetInContainer: (moduleName: string) =>
Expand Down Expand Up @@ -40,7 +48,9 @@ const errors = {
export const ModuleContainerErrors = errors;

// determines that a module should be configurable by default
export type BaseModuleType = TypedClass<Configurable<unknown>>;
export type BaseModuleType = TypedClass<
ChildContainerStartable & Configurable<unknown>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if we did a merged interface for this joint type and used it in ConfigurableModule implements ... as well

>;

// allows to specify what kind of modules can be passed into a container
export interface ModulesRecord<
Expand Down Expand Up @@ -83,12 +93,10 @@ export class ModuleContainer<
private static readonly moduleDecorationFrequency: Frequency = "Once";

// DI container holding all the registered modules
protected readonly container = container.createChildContainer();
private providedContainer?: DependencyContainer = undefined;

public constructor(public definition: ModuleContainerDefinition<Modules>) {
super();
// register all provided modules when the container is created
this.registerModules(definition.modules);
}

/**
Expand Down Expand Up @@ -127,6 +135,11 @@ export class ModuleContainer<
});
}

protected get container(): DependencyContainer {
this.assertContainerInitialized(this.providedContainer);
return this.providedContainer;
}

/**
* Assert that the iterated `moduleName` is of ModuleName type,
* otherwise it may be just string e.g. when modules are iterated over
Expand All @@ -148,6 +161,16 @@ export class ModuleContainer<
}
}

public assertContainerInitialized(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably start avoiding in-class assertion methods since we've learned recently they dont work outside of classes/instances themselves. But out of scope here.

container: DependencyContainer | undefined
): asserts container is DependencyContainer {
if (container === undefined) {
throw new Error(
"DependencyContainer not set. Be sure to only call DI-related function in create() and not inside the constructor."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract the error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if we could add this.constructor.name to the error, assuming it'll print the class that inherited the ModuleContainer

);
}
}

/**
* Register modules into the current container, and registers
* a respective resolution hook in order to decorate the module
Expand All @@ -162,9 +185,11 @@ export class ModuleContainer<

log.debug(`Registering module: ${moduleName}`);

const definitionEntry = modules[moduleName];

this.container.register(
moduleName,
{ useClass: modules[moduleName] },
{ useClass: definitionEntry },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By extracting this into a variable it throws a shorthand properties eslint error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we disable this rule? In my opinion it actually reduces readability in a lot of cases. Bcs renaming it useClass would remove information

{ lifecycle: Lifecycle.ContainerScoped }
);
this.onAfterModuleResolution(moduleName);
Expand Down Expand Up @@ -196,6 +221,16 @@ export class ModuleContainer<
});
}

protected registerClasses(modules: Record<string, TypedClass<unknown>>) {
Object.entries(modules).forEach(([moduleName, useClass]) => {
this.container.register(
moduleName,
{ useClass },
{ lifecycle: Lifecycle.ContainerScoped }
);
});
}

/**
* Provide additional configuration after the ModuleContainer was created.
*
Expand Down Expand Up @@ -234,7 +269,10 @@ export class ModuleContainer<
const isValidModuleInstance = instance instanceof moduleType;

if (!isValidModuleInstance) {
throw new Error("Incompatible module instance");
console.log(instance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

throw new Error(
`Incompatible module instance ("${moduleName}" not instanceof ${moduleType.name})`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract this error please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's your code 😂

);
}

return instance;
Expand Down Expand Up @@ -271,8 +309,30 @@ export class ModuleContainer<
throw errors.unableToDecorateModule(containedModuleName);
}
this.decorateModule(moduleName, containedModule);

containedModule.create(() => {
const container = this.container.createChildContainer();
container.reset();
rpanic marked this conversation as resolved.
Show resolved Hide resolved
return container;
});
},
{ frequency: ModuleContainer.moduleDecorationFrequency }
);
}

/**
* This is a placeholder for individual modules to override.
* This method will be called whenever the underlying container fully
* initialized
*/
public create(childContainerProvider: ChildContainerProvider): void {
this.providedContainer = childContainerProvider();
rpanic marked this conversation as resolved.
Show resolved Hide resolved

// register all provided modules when the container is created
this.registerModules(this.definition.modules);

this.registerValue({
ChildContainerProvider: () => this.container.createChildContainer(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be nicer if the injection token was childContainerProvider since its just a function, and not a class? But i get the follow up on the original interface name ChildContainerProvider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has more value to start our injection tokens consistently with a Uppercase letter. I know it is a function but we don't distinguish between services and primitives (yet). Let me know if you think differently

});
}
}
5 changes: 5 additions & 0 deletions packages/common/src/dependencyFactory/DependencyFactory.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { DependencyContainer, injectable, Lifecycle } from "tsyringe";

import { TypedClass } from "../types";
import { log } from "../log";

const errors = {
descriptorUndefined: () =>
Expand Down Expand Up @@ -56,6 +57,10 @@ export abstract class DependencyFactory {
},
{ lifecycle: Lifecycle.ContainerScoped }
);

log.debug(
`Registered dependency ${upperCaseKey} from factory ${this.constructor.name}`
);
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/common/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export * from "./config/ModuleContainer";
export * from "./config/ConfigurableModule";
export * from "./config/ChildContainerProvider";
export * from "./config/ChildContainerStartable";
export * from "./types";
export * from "./zkProgrammable/ZkProgrammable";
export * from "./zkProgrammable/ProvableMethodExecutionContext";
Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ export function dummyValue<Value>(
return valueType.fromFields(fields) as Value;
}

// eslint-disable-next-line @typescript-eslint/no-empty-function
export function noop(): void {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could have used lodash/noop 😂


export interface ToFieldable {
toFields: () => Field[];
}
3 changes: 1 addition & 2 deletions packages/module/src/method/runtimeMethod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ export function toWrappedMethod(
}

// Assert that the given transaction has the correct methodId
const methodIdResolver =
runtime.dependencyContainer.resolve<MethodIdResolver>("MethodIdResolver");
const { methodIdResolver } = runtime;
const thisMethodId = Field(methodIdResolver.getMethodId(name, methodName));
if (!thisMethodId.isConstant()) {
throw errors.fieldNotConstant("methodId");
Expand Down
30 changes: 21 additions & 9 deletions packages/module/src/runtime/Runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
ZkProgrammable,
PlainZkProgram,
WithZkProgrammable,
AreProofsEnabled,
AreProofsEnabled, ChildContainerProvider
} from "@proto-kit/common";
import {
MethodPublicOutput,
Expand All @@ -29,6 +29,7 @@ import { MethodIdFactory } from "../factories/MethodIdFactory";

import { RuntimeModule } from "./RuntimeModule";
import { MethodIdResolver } from "./MethodIdResolver";
import { RuntimeEnvironment } from "./RuntimeEnvironment";

/**
* Record of modules accepted by the Runtime module container.
Expand Down Expand Up @@ -190,12 +191,18 @@ export class RuntimeZkProgrammable<
@injectable()
export class Runtime<Modules extends RuntimeModulesRecord>
extends ModuleContainer<Modules>
implements WithZkProgrammable<undefined, MethodPublicOutput>
implements
WithZkProgrammable<undefined, MethodPublicOutput>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a part of RuntimeEnvironment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

RuntimeEnvironment
{
public static from<Modules extends RuntimeModulesRecord>(
definition: RuntimeDefinition<Modules>
) {
return new Runtime(definition);
): TypedClass<Runtime<Modules>> {
return class RuntimeScoped extends Runtime<Modules> {
public constructor() {
super(definition);
}
};
}

// runtime modules composed into a ZkProgram
Expand Down Expand Up @@ -223,15 +230,16 @@ export class Runtime<Modules extends RuntimeModulesRecord>

// eslint-disable-next-line no-warning-comments
// TODO Remove after changing DFs to type-based approach
public start() {
this.registerValue({
Runtime: this,
});
public create(childContainerProvider: ChildContainerProvider) {
super.create(childContainerProvider);

console.log("Registering create()");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀


this.registerDependencyFactories([MethodIdFactory]);
}

public get appChain(): AreProofsEnabled | undefined {
return this.container.resolve<AreProofsEnabled>("AppChain");
return this.container.resolve<AreProofsEnabled>("AreProofsEnabled");
}

public get stateService(): StateService {
Expand All @@ -242,6 +250,10 @@ export class Runtime<Modules extends RuntimeModulesRecord>
return this.stateServiceProviderInstance;
}

public get methodIdResolver(): MethodIdResolver {
return this.container.resolve<MethodIdResolver>("MethodIdResolver");
}

/**
* @returns The dependency injection container of this runtime
*/
Expand Down
15 changes: 15 additions & 0 deletions packages/module/src/runtime/RuntimeEnvironment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { AreProofsEnabled, ZkProgrammable } from "@proto-kit/common";
import {
MethodPublicOutput,
StateService,
StateServiceProvider,
} from "@proto-kit/protocol";
import { MethodIdResolver } from "./MethodIdResolver";

export interface RuntimeEnvironment {
get appChain(): AreProofsEnabled | undefined;
get stateService(): StateService;
get stateServiceProvider(): StateServiceProvider;
get methodIdResolver(): MethodIdResolver;
zkProgrammable: ZkProgrammable<undefined, MethodPublicOutput>;
}
3 changes: 2 additions & 1 deletion packages/module/src/runtime/RuntimeModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
RuntimeDefinition,
RuntimeModulesRecord,
} from "./Runtime";
import { RuntimeEnvironment } from "./RuntimeEnvironment";

const errors = {
inputDataNotSet: () => new Error("Input data for runtime execution not set"),
Expand Down Expand Up @@ -52,7 +53,7 @@ export class RuntimeModule<Config> extends ConfigurableModule<Config> {

public name?: string;

public runtime?: Runtime<RuntimeModulesRecord>;
public runtime?: RuntimeEnvironment;

public constructor() {
super();
Expand Down
2 changes: 1 addition & 1 deletion packages/module/test/modules/Balances.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe("balances", () => {
},
});

runtime.dependencyContainer.register("AppChain", {
runtime.dependencyContainer.register("AreProofsEnabled", {
useValue: {
areProofsEnabled: false,

Expand Down
2 changes: 1 addition & 1 deletion packages/module/test/modules/State.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe("transient state", () => {
},
});

runtime.dependencyContainer.register("AppChain", {
runtime.dependencyContainer.register("AreProofsEnabled", {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't runtime be using runtime.providedContainer which is provided from the AppChain? Or since Runtime isnt an AppChain module it can't receive a childContainerFactory?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, under the hood, .dependencyContainer returns providedContainer but does an additional assertion that it is set to throw a nice error in the case it is not

useValue: {
areProofsEnabled: false,

Expand Down
7 changes: 2 additions & 5 deletions packages/module/test/state/MockAsyncMerkleStore.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import {
AsyncMerkleTreeStore,
InMemoryMerkleTreeStorage,
noop,
} from "@proto-kit/protocol";
import { AsyncMerkleTreeStore, InMemoryMerkleTreeStorage } from "@proto-kit/protocol";
import { noop } from "@proto-kit/common";

export class MockAsyncMerkleTreeStore implements AsyncMerkleTreeStore {
public readonly store = new InMemoryMerkleTreeStorage();
Expand Down
Loading