BaseControllerV2 metadata is required for all the properties

[kanthesha]

Explanation

The StateMetadata type in BaseControllerV2 was intended to require each state property to have associated metadata. However, currently it doesn't require metadata for optional fields. This is problematic because all top-level state properties are expected to have metadata, and will throw an error at runtime if it's missing.

We should update the StateMetadata type to require metadata for all properties, including optional properties.

References

• Fixes [base-controller] Require metadata for optional state properties #4252

Changelog

@metamask/base-controller

• FIXED: Type of metadata in constructor params has been wrapped in Required to make sure metadata required for all properties, including optional properties.

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[kanthesha]

This can also be fixed with.

Suggested change

	const metadata = {
	const metadata: Required<StateMetadata<AuthenticationControllerState>> = {

Type has been removed to be inline with other v2 controllers.

[MajorLift]

Maybe we could update the StateMetadata type itself in base-controller? That way StateMetadata doesn't need to be wrapped in Required everywhere it's used.

diff --git a/packages/base-controller/src/BaseControllerV2.ts b/packages/base-controller/src/BaseControllerV2.ts
index bda31c8cb..eb245bb70 100644
--- a/packages/base-controller/src/BaseControllerV2.ts
+++ b/packages/base-controller/src/BaseControllerV2.ts
@@ -51,7 +51,7 @@ export type StateDeriver<T extends Json> = (value: T) => Json;
 // TODO: Either fix this lint violation or explain why it's necessary to ignore.
 // eslint-disable-next-line @typescript-eslint/naming-convention
 export type StateMetadata<T extends StateConstraint> = {
-  [P in keyof T]: StatePropertyMetadata<T[P]>;
+  [P in keyof T]-?: StatePropertyMetadata<T[P]>;
 };
 
 /**

[kanthesha]

Maybe we could update the StateMetadata type itself in base-controller? That way StateMetadata doesn't need to be wrapped in Required everywhere it's used.

diff --git a/packages/base-controller/src/BaseControllerV2.ts b/packages/base-controller/src/BaseControllerV2.ts
index bda31c8cb..eb245bb70 100644
--- a/packages/base-controller/src/BaseControllerV2.ts
+++ b/packages/base-controller/src/BaseControllerV2.ts
@@ -51,7 +51,7 @@ export type StateDeriver<T extends Json> = (value: T) => Json;
 // TODO: Either fix this lint violation or explain why it's necessary to ignore.
 // eslint-disable-next-line @typescript-eslint/naming-convention
 export type StateMetadata<T extends StateConstraint> = {
-  [P in keyof T]: StatePropertyMetadata<T[P]>;
+  [P in keyof T]-?: StatePropertyMetadata<T[P]>;
 };
 
 /**

Make sense. Included the suggestion in this commit.

[MajorLift]

LGTM!