Skip to content

Dry address comparison #31750

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Dry address comparison #31750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99d48d0
dry into isAddressWithParsedScopesInPermittedAccountIds
jiexi Apr 9, 2025
a44df57
fix import path
jiexi Apr 9, 2025
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
83 changes: 67 additions & 16 deletions shared/lib/multichain/chain-agnostic-permission-utils/caip-accounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@ import {
parseScopeString,
} from '@metamask/chain-agnostic-permission';
import type { InternalAccount } from '@metamask/keyring-internal-api';
import { CaipAccountId, parseCaipAccountId } from '@metamask/utils';
import {
CaipAccountAddress,
CaipAccountId,
CaipNamespace,
CaipReference,
parseCaipAccountId,
} from '@metamask/utils';

/**
* Gets all accounts from an array of scopes objects
Expand Down Expand Up @@ -48,32 +54,33 @@ export function getCaipAccountIdsFromCaip25CaveatValue(
}

/**
* Checks if an internal account is connected to any of the permitted accounts
* based on scope matching
* Checks if an address and list of parsed scopes are connected to any of
* the permitted accounts based on scope matching
*
* @param internalAccount - The internal account to check against permitted accounts
* @param permittedAccounts - Array of CAIP-10 account IDs that are permitted
* @returns True if the account is connected to any permitted account
* @param address - The CAIP account address to check against permitted accounts
* @param parsedAccountScopes - The list of parsed CAIP chain ID to check against permitted accounts
* @param permittedAccounts - Array of CAIP account IDs that are permitted
* @returns True if the address and any account scope is connected to any permitted account
*/
export function isInternalAccountInPermittedAccountIds(
internalAccount: InternalAccount,
function isAddressWithParsedScopesInPermittedAccountIds(
address: CaipAccountAddress,
parsedAccountScopes: {
namespace?: CaipNamespace;
reference?: CaipReference;
}[],
permittedAccounts: CaipAccountId[],
): boolean {
if (!internalAccount || !permittedAccounts.length) {
) {
if (!address || !parsedAccountScopes.length || !permittedAccounts.length) {
return false;
}

const parsedInteralAccountScopes = internalAccount.scopes.map((scope) => {
return parseScopeString(scope);
});

return permittedAccounts.some((account) => {
const parsedPermittedAccount = parseCaipAccountId(account);

return parsedInteralAccountScopes.some(({ namespace, reference }) => {
return parsedAccountScopes.some(({ namespace, reference }) => {
if (
namespace !== parsedPermittedAccount.chain.namespace ||
internalAccount.address !== parsedPermittedAccount.address
address !== parsedPermittedAccount.address
) {
return false;
}
Expand All @@ -85,3 +92,47 @@ export function isInternalAccountInPermittedAccountIds(
});
});
}

/**
* Checks if an internal account is connected to any of the permitted accounts
* based on scope matching
*
* @param internalAccount - The internal account to check against permitted accounts
* @param permittedAccounts - Array of CAIP account IDs that are permitted
* @returns True if the account is connected to any permitted account
*/
export function isInternalAccountInPermittedAccountIds(
internalAccount: InternalAccount,
permittedAccounts: CaipAccountId[],
): boolean {
const parsedInteralAccountScopes = internalAccount.scopes.map((scope) => {
return parseScopeString(scope);
});

return isAddressWithParsedScopesInPermittedAccountIds(
internalAccount.address,
parsedInteralAccountScopes,
permittedAccounts,
);
}

/**
* Checks if an CAIP account ID is connected to any of the permitted accounts
* based on scope matching
*
* @param accountId - The CAIP account ID to check against permitted accounts
* @param permittedAccounts - Array of CAIP account IDs that are permitted
* @returns True if the account is connected to any permitted account
*/
export function isAccountIdInPermittedAccountIds(
Comment on lines +120 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the key thing to highlight about this function is that the "permittedAccounts" will contain 0 as references, which is why we can't just do a simple includes() check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

accountId: CaipAccountId,
permittedAccounts: CaipAccountId[],
): boolean {
const { address, chain } = parseCaipAccountId(accountId);

return isAddressWithParsedScopesInPermittedAccountIds(
address,
[chain],
permittedAccounts,
);
}
37 changes: 7 additions & 30 deletions ui/components/multichain/pages/review-permissions-page/review-permissions-page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
MergedInternalAccountWithCaipAccountId,
} from '../../../../selectors/selectors.types';
import { CAIP_FORMATTED_EVM_TEST_CHAINS } from '../../../../../shared/constants/network';
import { isAccountIdInPermittedAccountIds } from '../../../../../shared/lib/multichain/chain-agnostic-permission-utils/caip-accounts';
import { SiteCell } from './site-cell/site-cell';

export const ReviewPermissions = () => {
Expand Down Expand Up @@ -205,39 +206,15 @@ export const ReviewPermissions = () => {
// TODO: we should refactor addPermittedAccounts to accept CaipAccountIds
dispatch(addPermittedAccounts(activeTabOrigin, addresses));

connectedAccountAddresses.forEach((connectedAddress: string) => {
// TODO: seems like similar logic to selector logic in ui/index.js
// See if we can DRY this
const parsedConnectedAddress = parseCaipAccountId(
connectedAddress as CaipAccountId,
connectedAccountAddresses.forEach((connectedAddress: CaipAccountId) => {
const includesCaipAccountId = isAccountIdInPermittedAccountIds(
connectedAddress,
caipAccountIds,
);

const includesCaipAccountId = parsedCaipAccountIds.some(
(parsedAddress) => {
if (
parsedConnectedAddress.chain.namespace !==
parsedAddress.chain.namespace ||
parsedConnectedAddress.address !== parsedAddress.address
) {
return false;
}

return (
parsedAddress.chain.reference === '0' ||
parsedAddress.chain.reference ===
parsedConnectedAddress.chain.reference
);
},
);

if (!includesCaipAccountId) {
const { address } = parseCaipAccountId(connectedAddress);
// TODO: we should refactor removePermittedAccount to accept CaipAccountIds
dispatch(
removePermittedAccount(
activeTabOrigin,
parsedConnectedAddress.address,
),
);
dispatch(removePermittedAccount(activeTabOrigin, address));
}
});

Expand Down
Loading