Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [UNRELEASED]

### Bug Fixes

- **`SafeAccountV1_5_0_M_0_3_0.initializeNewAccount` returns the right runtime type.** The subclass factory delegated to `SafeAccountV0_3_0.initializeNewAccount`, which hard-coded `new SafeAccountV0_3_0(...)`, so the returned instance failed `instanceof SafeAccountV1_5_0_M_0_3_0` and lost subclass behavior. The base factory now instantiates polymorphically through `new this(...)`, so any subclass (including consumer-defined ones) gets its own type back. (#116)

## 0.4.0

### New Features
Expand Down
16 changes: 11 additions & 5 deletions src/account/Safe/SafeAccountV0_3_0.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,22 @@ export class SafeAccountV0_3_0 extends SafeAccount {
* The account address is deterministically computed but not yet deployed on-chain.
* The first UserOperation sent will deploy it automatically via factory data.
*
* Instantiates through `new this(...)`, so subclasses calling this
* factory (directly or via `super`) get an instance of the subclass,
* not a plain SafeAccountV0_3_0.
*
* @param owners - Array of owner signers (at least one required)
* @param overrides - Override default initialization values
* @returns A SafeAccountV0_3_0 instance with factory data set for deployment
* @returns An instance of the calling class with factory data set for deployment
*
* @example
* const smartAccount = SafeAccountV0_3_0.initializeNewAccount(["0xOwnerAddress"]);
*/
public static initializeNewAccount(
public static initializeNewAccount<T extends typeof SafeAccountV0_3_0>(
this: T,
owners: Signer[],
overrides: InitCodeOverrides = {},
): SafeAccountV0_3_0 {
): InstanceType<T> {
let isInitWebAuthn = false;
let x = 0n;
let y = 0n;
Expand All @@ -122,7 +127,8 @@ export class SafeAccountV0_3_0 extends SafeAccount {
overrides.safeModuleSetupAddress ?? SafeAccountV0_3_0.DEFAULT_SAFE_MODULE_SETUP_ADDRESS,
);

const safe = new SafeAccountV0_3_0(accountAddress, {
// biome-ignore lint/complexity/noThisInStatic: polymorphic factory; subclasses must get their own type back
const safe: SafeAccountV0_3_0 = new this(accountAddress, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle unbound factory calls

When JavaScript consumers cache or destructure this public factory (for example, const init = SafeAccountV0_3_0.initializeNewAccount; init([owner])), the static this value is undefined, so new this(...) now throws TypeError: this is not a constructor. The previous hard-coded constructor path worked in that scenario, so this is a runtime breaking change for existing JS/CommonJS callers that use the factory as a callback or extracted function.

Useful? React with 👍 / 👎.

safe4337ModuleAddress: overrides.safe4337ModuleAddress,
entrypointAddress: overrides.entrypointAddress,
onChainIdentifierParams: overrides.onChainIdentifierParams,
Expand All @@ -137,7 +143,7 @@ export class SafeAccountV0_3_0 extends SafeAccount {
safe.y = y;
}

return safe;
return safe as InstanceType<T>;
}

/**
Expand Down
11 changes: 8 additions & 3 deletions src/account/Safe/SafeAccountV1_5_0_M_0_3_0.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,11 @@ export class SafeAccountV1_5_0_M_0_3_0 extends SafeAccountV0_3_0 {
* @example
* const smartAccount = SafeAccountV1_5_0_M_0_3_0.initializeNewAccount(["0xOwnerAddress"]);
*/
public static initializeNewAccount(
public static initializeNewAccount<T extends typeof SafeAccountV0_3_0>(
this: T,
owners: Signer[],
overrides: InitCodeOverrides = {},
): SafeAccountV1_5_0_M_0_3_0 {
): InstanceType<T> {
const modOverrides = {
...overrides,
safeAccountSingleton: overrides.safeAccountSingleton ?? Safe_L2_V1_5_0,
Expand All @@ -111,7 +112,11 @@ export class SafeAccountV1_5_0_M_0_3_0 extends SafeAccountV0_3_0 {
overrides.eip7212WebAuthnContractVerifierForSharedSigner ??
SafeAccountV1_5_0_M_0_3_0.DEFAULT_WEB_AUTHN_CONTRACT_VERIFIER,
};
return SafeAccountV0_3_0.initializeNewAccount(owners, modOverrides);
// `super` keeps `this` bound to the calling class, so the base
// factory's `new this(...)` constructs an instance of this class
// (SafeAccountV1_5_0_M_0_3_0 or a subclass), not SafeAccountV0_3_0.
// biome-ignore lint/complexity/noThisInStatic: polymorphic factory dispatch through super
return super.initializeNewAccount(owners, modOverrides) as InstanceType<T>;
}

/**
Expand Down
32 changes: 32 additions & 0 deletions test/safe/initializeNewAccountRuntimeType.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Unit tests for issue #116: SafeAccountV1_5_0_M_0_3_0.initializeNewAccount
// must return a SafeAccountV1_5_0_M_0_3_0 instance at runtime, not a plain
// SafeAccountV0_3_0. No network required.

const ak = require("../../dist/index.cjs");

const OWNER = "0x2222222222222222222222222222222222222222";

describe("initializeNewAccount runtime type (#116)", () => {
test("SafeAccountV1_5_0_M_0_3_0.initializeNewAccount returns a SafeAccountV1_5_0_M_0_3_0", () => {
const account = ak.SafeAccountV1_5_0_M_0_3_0.initializeNewAccount([OWNER]);
expect(account).toBeInstanceOf(ak.SafeAccountV1_5_0_M_0_3_0);
});

test("instance address matches the class's createAccountAddress (v1.5.0 singleton)", () => {
const account = ak.SafeAccountV1_5_0_M_0_3_0.initializeNewAccount([OWNER]);
expect(account.accountAddress).toBe(ak.SafeAccountV1_5_0_M_0_3_0.createAccountAddress([OWNER]));
});

test("SafeAccountV0_3_0.initializeNewAccount still returns a plain SafeAccountV0_3_0", () => {
const account = ak.SafeAccountV0_3_0.initializeNewAccount([OWNER]);
expect(account).toBeInstanceOf(ak.SafeAccountV0_3_0);
expect(account).not.toBeInstanceOf(ak.SafeAccountV1_5_0_M_0_3_0);
expect(account.accountAddress).toBe(ak.SafeAccountV0_3_0.createAccountAddress([OWNER]));
});

test("a consumer subclass inheriting the factory gets its own type back", () => {
class CustomSafe extends ak.SafeAccountV0_3_0 {}
const account = CustomSafe.initializeNewAccount([OWNER]);
expect(account).toBeInstanceOf(CustomSafe);
});
});
Loading