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

chore: unblock account create limit #493

Merged
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 0 additions & 8 deletions packages/starknet-snap/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ export type SnapConfig = {
txnsInLastNumOfDays: number;
};
};
account: {
maxAccountToCreate: number;
};
};

export enum DataClient {
Expand All @@ -63,11 +60,6 @@ export const Config: SnapConfig = {
txnsInLastNumOfDays: 10,
},
},

account: {
maxAccountToCreate: 2,
},

// eslint-disable-next-line no-restricted-globals
rpcApiKey: process.env.DIN_API_KEY ?? '',

Expand Down
5 changes: 0 additions & 5 deletions packages/starknet-snap/src/state/__tests__/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,16 +100,11 @@ export const mockAccountStateManager = () => {
AccountStateManager.prototype,
'upsertAccount',
);
const isMaxAccountLimitExceededSpy = jest.spyOn(
AccountStateManager.prototype,
'isMaxAccountLimitExceeded',
);

return {
getAccountSpy,
getNextIndexSpy,
upsertAccountSpy,
isMaxAccountLimitExceededSpy,
};
};

Expand Down
31 changes: 0 additions & 31 deletions packages/starknet-snap/src/state/account-state-manager.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { constants } from 'starknet';

import type { StarknetAccount } from '../__tests__/helper';
import { Config } from '../config';
import {
generateMainnetAccounts,
generateTestnetAccounts,
Expand Down Expand Up @@ -210,36 +209,6 @@ describe('AccountStateManager', () => {
});
});

describe('isMaxAccountLimitExceeded', () => {
it('returns true if the account limit is reached', async () => {
const accounts = await generateTestnetAccounts(
Config.account.maxAccountToCreate + 1,
);
await mockStateWithMainnetAccounts(accounts);

const stateManager = new AccountStateManager();
const result = await stateManager.isMaxAccountLimitExceeded({
chainId: testnetChainId,
});

expect(result).toBe(true);
});

it('returns false if the account limit is not reached', async () => {
const accounts = await generateTestnetAccounts(
Config.account.maxAccountToCreate,
);
await mockStateWithMainnetAccounts(accounts);

const stateManager = new AccountStateManager();
const result = await stateManager.isMaxAccountLimitExceeded({
chainId: testnetChainId,
});

expect(result).toBe(false);
});
});

describe('getCurrentAccount', () => {
const setupGetCurrentAccountTest = async () => {
const accounts = await generateTestnetAccounts();
Expand Down
23 changes: 0 additions & 23 deletions packages/starknet-snap/src/state/account-state-manager.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Config } from '../config';
import type { AccContract, SnapState } from '../types/snapState';
import type { IFilter } from './filter';
import {
Expand Down Expand Up @@ -217,28 +216,6 @@ export class AccountStateManager extends StateManager<AccContract> {
}
}

/**
* Determines whether max account limit exceeded.
*
* @param params - The parameters for checking the max account limit.
* @param params.chainId - The chain ID.
* @param [state] - The optional SnapState object.
* @returns A Promise that resolves to a boolean indicating whether the max account limit is exceeded.
*/
async isMaxAccountLimitExceeded(
{
chainId,
}: {
chainId: string;
},
state?: SnapState,
): Promise<boolean> {
return (
(await this.list([new ChainIdFilter([chainId])], undefined, state))
.length > Config.account.maxAccountToCreate
);
}

/**
* Gets the current account for the specified chain ID.
*
Expand Down
11 changes: 0 additions & 11 deletions packages/starknet-snap/src/utils/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
UserRejectedRequestError,
} from '@metamask/snaps-sdk';

import { Config } from '../config';
import { createWalletRpcErrorWrapper, WalletRpcErrorCode } from './error';

// Extend SnapError to allow error message visible to client
Expand Down Expand Up @@ -50,16 +49,6 @@ export class AccountDiscoveryError extends SnapError {
}
}

export class MaxAccountLimitExceededError extends SnapError {
constructor(message?: string) {
super(
message ??
`Maximum number of accounts reached: ${Config.account.maxAccountToCreate}`,
createWalletRpcErrorWrapper(WalletRpcErrorCode.Unknown),
);
}
}

export class ContractReadError extends SnapError {
constructor(message: string) {
super(message, createWalletRpcErrorWrapper(WalletRpcErrorCode.Unknown));
Expand Down
54 changes: 3 additions & 51 deletions packages/starknet-snap/src/wallet/account/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,10 @@ import { generateMnemonic } from 'bip39';

import { AccountService } from '.';
import { generateKeyDeriver } from '../../__tests__/helper';
import {
mockAccountStateManager,
mockState,
} from '../../state/__tests__/helper';
import { mockAccountStateManager } from '../../state/__tests__/helper';
import { AccountStateManager } from '../../state/account-state-manager';
import { STARKNET_SEPOLIA_TESTNET_NETWORK } from '../../utils/constants';
import {
AccountNotFoundError,
MaxAccountLimitExceededError,
} from '../../utils/exceptions';
import { AccountNotFoundError } from '../../utils/exceptions';
import { createAccountService } from '../../utils/factory';
import * as snapUtils from '../../utils/snap';
import {
Expand Down Expand Up @@ -63,23 +57,17 @@ describe('AccountService', () => {
);
await mockSnapDeriver(mnemonicString);

const {
getNextIndexSpy,
isMaxAccountLimitExceededSpy,
upsertAccountSpy,
} = mockAccountStateManager();
const { getNextIndexSpy, upsertAccountSpy } = mockAccountStateManager();

mockAccountContractReader({});

getNextIndexSpy.mockResolvedValue(hdIndex);
isMaxAccountLimitExceededSpy.mockResolvedValue(false);

return {
upsertAccountSpy,
getNextIndexSpy,
getCairoContractSpy,
account: accountObj,
isMaxAccountLimitExceededSpy,
};
};

Expand Down Expand Up @@ -132,42 +120,6 @@ describe('AccountService', () => {
expect(result).toHaveProperty('hdIndex', hdIndex);
expect(result).toHaveProperty('addressSalt', account.publicKey);
});

it('throws `MaxAccountLimitExceededError` error if the account to derive reach the maximum', async () => {
const { isMaxAccountLimitExceededSpy } =
await setupDeriveAccountByIndexTest(0);
isMaxAccountLimitExceededSpy.mockResolvedValue(true);

const service = createAccountService(network);

await expect(service.deriveAccountByIndex()).rejects.toThrow(
MaxAccountLimitExceededError,
);
});

it('does not modify the state if an error has thrown', async () => {
const { setDataSpy } = await mockState({});
// mockAccountStateManager is only returning the spies,
// it will not mock the function to return a value.
const { isMaxAccountLimitExceededSpy } = mockAccountStateManager();

const mnemonicString = generateMnemonic();
await mockDeriveAccount(0, mnemonicString);
await mockSnapDeriver(mnemonicString);
mockAccountContractReader({});

// A `MaxAccountLimitExceededError` will be thrown when `isMaxAccountLimitExceeded` is true.
// Since this checking is placed at the end of the function,
// it is the best way to test if the state is not modified if an error occurs.
isMaxAccountLimitExceededSpy.mockResolvedValue(true);

const service = createAccountService(network);

await expect(service.deriveAccountByIndex()).rejects.toThrow(
MaxAccountLimitExceededError,
);
expect(setDataSpy).not.toHaveBeenCalled();
});
});

describe('deriveAccountFromAddress', () => {
Expand Down
19 changes: 2 additions & 17 deletions packages/starknet-snap/src/wallet/account/service.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { AccountStateManager } from '../../state/account-state-manager';
import type { Network } from '../../types/snapState';
import { getBip44Deriver } from '../../utils';
import {
AccountNotFoundError,
MaxAccountLimitExceededError,
} from '../../utils/exceptions';
import { AccountNotFoundError } from '../../utils/exceptions';
import { Account } from './account';
import { AccountContractDiscovery } from './discovery';
import { AccountKeyPair } from './keypair';
Expand Down Expand Up @@ -53,7 +50,7 @@ export class AccountService {
const { chainId } = this.network;

// use `withTransaction` to ensure that the state is not modified if an error occurs.
return this.accountStateMgr.withTransaction(async (state) => {
return this.accountStateMgr.withTransaction(async () => {
let hdIndex = index;
if (hdIndex === undefined) {
hdIndex = await this.accountStateMgr.getNextIndex(chainId);
Expand Down Expand Up @@ -82,18 +79,6 @@ export class AccountService {

await this.accountStateMgr.upsertAccount(await account.serialize());

// FIXME: this is a convenience way to check if the account limit has been exceeded at the last line of the code. However, it is possible to improve if we can check it before the account is derived.
if (
await this.accountStateMgr.isMaxAccountLimitExceeded(
{
chainId,
},
state,
)
) {
throw new MaxAccountLimitExceededError();
}

return account;
});
}
Expand Down
Loading