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>
10 changes: 5 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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

export interface ChildContainerCreatable {
create: (childContainerProvider: ChildContainerProvider) => void;
}
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;
}
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 type { BaseModuleInstanceType } from "./ModuleContainer";

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 BaseModuleInstanceType
{
/**
* 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
81 changes: 74 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 { ChildContainerCreatable } from "./ChildContainerCreatable";

const errors = {
configNotSetInContainer: (moduleName: string) =>
Expand Down Expand Up @@ -35,12 +43,26 @@ const errors = {
attempting to inject a dependency that is not registered
as a runtime module for this chain: ${name}`
),

dependencyContainerNotSet: (className: string) =>
new Error(
`DependencyContainer not set. Be sure to only call DI-related function in create() and not inside the constructor. (${className})`
),

validModuleInstance: (moduleName: string, moduleTypeName: string) =>
new Error(
`Incompatible module instance ("${moduleName}" not instanceof ${moduleTypeName})`
),
};

export const ModuleContainerErrors = errors;

export interface BaseModuleInstanceType
extends ChildContainerCreatable,
Configurable<unknown> {}

// determines that a module should be configurable by default
export type BaseModuleType = TypedClass<Configurable<unknown>>;
export type BaseModuleType = TypedClass<BaseModuleInstanceType>;

// allows to specify what kind of modules can be passed into a container
export interface ModulesRecord<
Expand Down Expand Up @@ -83,12 +105,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 +147,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 +173,14 @@ 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 errors.dependencyContainerNotSet(this.constructor.name);
}
}

/**
* Register modules into the current container, and registers
* a respective resolution hook in order to decorate the module
Expand All @@ -162,9 +195,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 +231,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 +279,7 @@ export class ModuleContainer<
const isValidModuleInstance = instance instanceof moduleType;

if (!isValidModuleInstance) {
throw new Error("Incompatible module instance");
throw errors.validModuleInstance(moduleName, moduleType.name);
}

return instance;
Expand Down Expand Up @@ -271,8 +316,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/ChildContainerCreatable";
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
26 changes: 17 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,16 @@ export class RuntimeZkProgrammable<
@injectable()
export class Runtime<Modules extends RuntimeModulesRecord>
extends ModuleContainer<Modules>
implements WithZkProgrammable<undefined, MethodPublicOutput>
implements 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 +228,14 @@ 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);

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 +246,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
19 changes: 19 additions & 0 deletions packages/module/src/runtime/RuntimeEnvironment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import {
AreProofsEnabled,
WithZkProgrammable,
ZkProgrammable,
} from "@proto-kit/common";
import {
MethodPublicOutput,
StateService,
StateServiceProvider,
} from "@proto-kit/protocol";
import { MethodIdResolver } from "./MethodIdResolver";

export interface RuntimeEnvironment
extends WithZkProgrammable<undefined, MethodPublicOutput> {
get appChain(): AreProofsEnabled | undefined;
get stateService(): StateService;
get stateServiceProvider(): StateServiceProvider;
get methodIdResolver(): MethodIdResolver;
}
Loading