From 80b64973c88e1d64e30cc97b5049d5ef2a9af46f Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Wed, 25 Sep 2024 12:36:59 +0200 Subject: [PATCH 1/5] feat: Add the `size` prop to `Heading` (#2759) This PR adds an optional `size` prop to the `Heading` component. I can be `md` or `lg`. --- .../packages/browserify-plugin/snap.manifest.json | 2 +- .../examples/packages/browserify/snap.manifest.json | 2 +- .../snaps-sdk/src/jsx/components/Heading.test.tsx | 13 +++++++++++++ packages/snaps-sdk/src/jsx/components/Heading.ts | 5 +++++ packages/snaps-sdk/src/jsx/components/Text.ts | 4 ++++ packages/snaps-sdk/src/jsx/validation.test.tsx | 2 +- packages/snaps-sdk/src/jsx/validation.ts | 1 + 7 files changed, 26 insertions(+), 3 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index 2214c8ea2e..0d55c9fe57 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "Rvk4nDRt8bofy5oUtavVGwVB0pnZ7BdsmO9V315Qs+w=", + "shasum": "Kn7XJHg0P1YYLU+sZF9pon4NXLadxLmUETLuLAn1qkw=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index e8a3a4c059..140638e4ba 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "3LsbiHzT2I8gHJPQCu6JbTiKKCy5XA2VQQdW9Qkn3XU=", + "shasum": "xvq90PFxbF6rSf1t2+LeGCP5oeby0u7bobCT2ySxpF0=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snaps-sdk/src/jsx/components/Heading.test.tsx b/packages/snaps-sdk/src/jsx/components/Heading.test.tsx index cc2add3b04..6ead885b8c 100644 --- a/packages/snaps-sdk/src/jsx/components/Heading.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/Heading.test.tsx @@ -41,4 +41,17 @@ describe('Heading', () => { }, }); }); + + it('renders a heading with a `lg` size', () => { + const result = Foo; + + expect(result).toStrictEqual({ + type: 'Heading', + key: null, + props: { + children: 'Foo', + size: 'lg', + }, + }); + }); }); diff --git a/packages/snaps-sdk/src/jsx/components/Heading.ts b/packages/snaps-sdk/src/jsx/components/Heading.ts index ee3ff21067..93e7a91f39 100644 --- a/packages/snaps-sdk/src/jsx/components/Heading.ts +++ b/packages/snaps-sdk/src/jsx/components/Heading.ts @@ -5,9 +5,11 @@ import { createSnapComponent } from '../component'; * The props of the {@link Heading} component. * * @property children - The text to display in the heading. + * @property size - The size of the heading. */ type HeadingProps = { children: StringElement; + size?: 'md' | 'lg' | undefined; }; const TYPE = 'Heading'; @@ -17,9 +19,12 @@ const TYPE = 'Heading'; * * @param props - The props of the component. * @param props.children - The text to display in the heading. + * @param props.size - The size of the heading. * @returns A heading element. * @example * Hello world! + * @example + * Hello world! */ export const Heading = createSnapComponent(TYPE); diff --git a/packages/snaps-sdk/src/jsx/components/Text.ts b/packages/snaps-sdk/src/jsx/components/Text.ts index 42bee0170a..e3eb8144f7 100644 --- a/packages/snaps-sdk/src/jsx/components/Text.ts +++ b/packages/snaps-sdk/src/jsx/components/Text.ts @@ -26,6 +26,8 @@ export type TextColors = * The props of the {@link Text} component. * * @property children - The text to display. + * @property alignment - The alignment of the text. + * @property color - The color of the text. */ export type TextProps = { children: TextChildren; @@ -39,6 +41,8 @@ const TYPE = 'Text'; * A text component, which is used to display text. * * @param props - The props of the component. + * @param props.alignment - The alignment of the text. + * @param props.color - The color of the text. * @param props.children - The text to display. * @returns A text element. * @example diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index c0ec8df9a6..60dd9cd0d6 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -1010,7 +1010,7 @@ describe('SelectorStruct', () => { }); describe('HeadingStruct', () => { - it.each([Hello])( + it.each([Hello, Hello])( 'validates a heading element', (value) => { expect(is(value, HeadingStruct)).toBe(true); diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index 2437dfd7cc..dcd4a9e52d 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -605,6 +605,7 @@ export const ValueStruct: Describe = element('Value', { */ export const HeadingStruct: Describe = element('Heading', { children: StringElementStruct, + size: optional(nullUnion([literal('md'), literal('lg')])), }); /** From 0014ff6d7566dbc2945600db740d0a90f818b2d8 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Wed, 25 Sep 2024 14:53:13 +0200 Subject: [PATCH 2/5] Allow `Link` in `Row` and `Address` in `Link` (#2761) This PR allows two things: - `Link` to be a children of `Row` - `Address` to be a children of `Link` Fixes: #2757 --- .../browserify-plugin/snap.manifest.json | 2 +- .../packages/browserify/snap.manifest.json | 2 +- .../src/jsx/components/Link.test.tsx | 22 +++++++++++++ packages/snaps-sdk/src/jsx/components/Link.ts | 7 +++- .../snaps-sdk/src/jsx/components/Row.test.tsx | 33 +++++++++++++++++++ packages/snaps-sdk/src/jsx/components/Row.ts | 4 ++- .../snaps-sdk/src/jsx/validation.test.tsx | 8 +++++ packages/snaps-sdk/src/jsx/validation.ts | 16 +++++++-- 8 files changed, 88 insertions(+), 6 deletions(-) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index 0d55c9fe57..ef573ca559 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "Kn7XJHg0P1YYLU+sZF9pon4NXLadxLmUETLuLAn1qkw=", + "shasum": "OKpRMRDPOSMb+v1pNizzzKpUXrZIBLMJqXCh9xR3rFQ=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index 140638e4ba..601709e1ac 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "xvq90PFxbF6rSf1t2+LeGCP5oeby0u7bobCT2ySxpF0=", + "shasum": "9olSQCt4yqefE+yiWvkXwsB966J81losYyHKyLaVgKI=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snaps-sdk/src/jsx/components/Link.test.tsx b/packages/snaps-sdk/src/jsx/components/Link.test.tsx index 31752f1a07..dcb2839a9f 100644 --- a/packages/snaps-sdk/src/jsx/components/Link.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/Link.test.tsx @@ -1,3 +1,4 @@ +import { Address } from './Address'; import { Icon } from './Icon'; import { Link } from './Link'; @@ -49,6 +50,27 @@ describe('Link', () => { }); }); + it('renders a link with an Address', () => { + const result = ( + +
+ + ); + + expect(result).toStrictEqual({ + type: 'Link', + key: null, + props: { + href: 'https://example.com', + children: { + type: 'Address', + key: null, + props: { address: '0x1234567890123456789012345678901234567890' }, + }, + }, + }); + }); + it('renders a link with a conditional value', () => { const result = ( diff --git a/packages/snaps-sdk/src/jsx/components/Link.ts b/packages/snaps-sdk/src/jsx/components/Link.ts index b9698536f6..9618a8236e 100644 --- a/packages/snaps-sdk/src/jsx/components/Link.ts +++ b/packages/snaps-sdk/src/jsx/components/Link.ts @@ -1,5 +1,6 @@ import type { SnapsChildren } from '../component'; import { createSnapComponent } from '../component'; +import type { AddressElement } from './Address'; import type { StandardFormattingElement } from './formatting'; import { type IconElement } from './Icon'; import { type ImageElement } from './Image'; @@ -8,7 +9,11 @@ import { type ImageElement } from './Image'; * The children of the {@link Link} component. */ export type LinkChildren = SnapsChildren< - string | StandardFormattingElement | IconElement | ImageElement + | string + | StandardFormattingElement + | IconElement + | ImageElement + | AddressElement >; /** diff --git a/packages/snaps-sdk/src/jsx/components/Row.test.tsx b/packages/snaps-sdk/src/jsx/components/Row.test.tsx index aa0116e54f..56c1e3c146 100644 --- a/packages/snaps-sdk/src/jsx/components/Row.test.tsx +++ b/packages/snaps-sdk/src/jsx/components/Row.test.tsx @@ -1,5 +1,6 @@ import { Address } from './Address'; import { Image } from './Image'; +import { Link } from './Link'; import { Row } from './Row'; import { Text } from './Text'; @@ -73,4 +74,36 @@ describe('Row', () => { }, }); }); + + it('renders a row with a Link', () => { + const result = ( + + +
+ + + ); + + expect(result).toStrictEqual({ + type: 'Row', + key: null, + props: { + label: 'Foo', + children: { + type: 'Link', + key: null, + props: { + href: 'https://example.com', + children: { + type: 'Address', + key: null, + props: { + address: '0x1234567890123456789012345678901234567890', + }, + }, + }, + }, + }, + }); + }); }); diff --git a/packages/snaps-sdk/src/jsx/components/Row.ts b/packages/snaps-sdk/src/jsx/components/Row.ts index a23672ab44..b64511150f 100644 --- a/packages/snaps-sdk/src/jsx/components/Row.ts +++ b/packages/snaps-sdk/src/jsx/components/Row.ts @@ -1,6 +1,7 @@ import { createSnapComponent } from '../component'; import type { AddressElement } from './Address'; import type { ImageElement } from './Image'; +import type { LinkElement } from './Link'; import type { TextElement } from './Text'; import type { ValueElement } from './Value'; @@ -11,7 +12,8 @@ export type RowChildren = | AddressElement | ImageElement | TextElement - | ValueElement; + | ValueElement + | LinkElement; /** * The props of the {@link Row} component. diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index 60dd9cd0d6..c58e462d1d 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -1078,6 +1078,9 @@ describe('LinkStruct', () => { , + +
+ , ])('validates a link element', (value) => { expect(is(value, LinkStruct)).toBe(true); }); @@ -1100,6 +1103,11 @@ describe('LinkStruct', () => { alt , + + +
+ + , ])('does not validate "%p"', (value) => { expect(is(value, LinkStruct)).toBe(false); }); diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index dcd4a9e52d..8da02b5080 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -613,7 +613,13 @@ export const HeadingStruct: Describe = element('Heading', { */ export const LinkStruct: Describe = element('Link', { href: string(), - children: children([FormattingStruct, string(), IconStruct, ImageStruct]), + children: children([ + FormattingStruct, + string(), + IconStruct, + ImageStruct, + AddressStruct, + ]), }); /** @@ -691,7 +697,13 @@ export const TooltipStruct: Describe = element('Tooltip', { */ export const RowStruct: Describe = element('Row', { label: string(), - children: typedUnion([AddressStruct, ImageStruct, TextStruct, ValueStruct]), + children: typedUnion([ + AddressStruct, + ImageStruct, + TextStruct, + ValueStruct, + LinkStruct, + ]), variant: optional( nullUnion([literal('default'), literal('warning'), literal('critical')]), ), From 73d00a0e16b8f48733fadd539d10f7e784a589c6 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Thu, 26 Sep 2024 15:03:20 +0200 Subject: [PATCH 3/5] Add `AccountSelector` component (#2764) This PR adds an `AccountSelector` component. Fixes: #2752 --- .../browserify-plugin/snap.manifest.json | 2 +- .../packages/browserify/snap.manifest.json | 2 +- .../components/form/AccountSelector.test.tsx | 25 +++++++ .../jsx/components/form/AccountSelector.ts | 52 +++++++++++++++ .../src/jsx/components/form/index.ts | 3 + .../snaps-sdk/src/jsx/validation.test.tsx | 65 +++++++++++++++++++ packages/snaps-sdk/src/jsx/validation.ts | 20 ++++++ 7 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 packages/snaps-sdk/src/jsx/components/form/AccountSelector.test.tsx create mode 100644 packages/snaps-sdk/src/jsx/components/form/AccountSelector.ts diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index ef573ca559..5fe60b987f 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "OKpRMRDPOSMb+v1pNizzzKpUXrZIBLMJqXCh9xR3rFQ=", + "shasum": "+w62Op5ur4nVLmQ0uKA0IsAQN2hkKOsgSm4VK9jxxYY=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index 601709e1ac..ae76aea210 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "9olSQCt4yqefE+yiWvkXwsB966J81losYyHKyLaVgKI=", + "shasum": "SL7kg2vwhtpuzNvOx7uQZNZbccRgfFtTi0xZdEVEP0s=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snaps-sdk/src/jsx/components/form/AccountSelector.test.tsx b/packages/snaps-sdk/src/jsx/components/form/AccountSelector.test.tsx new file mode 100644 index 0000000000..7904e60cbf --- /dev/null +++ b/packages/snaps-sdk/src/jsx/components/form/AccountSelector.test.tsx @@ -0,0 +1,25 @@ +import { AccountSelector } from './AccountSelector'; + +describe('AccountSelector', () => { + it('returns an account selector element', () => { + const result = ( + + ); + + expect(result).toStrictEqual({ + type: 'AccountSelector', + props: { + name: 'account', + title: 'From account', + chainId: 'bip122:p2wpkh', + selectedAddress: 'bc1qc8dwyqua9elc3mzcxk93c70kjz8tcc92x0a8a6', + }, + key: null, + }); + }); +}); diff --git a/packages/snaps-sdk/src/jsx/components/form/AccountSelector.ts b/packages/snaps-sdk/src/jsx/components/form/AccountSelector.ts new file mode 100644 index 0000000000..78f54cf63b --- /dev/null +++ b/packages/snaps-sdk/src/jsx/components/form/AccountSelector.ts @@ -0,0 +1,52 @@ +import type { CaipAccountAddress, CaipChainId } from '@metamask/utils'; + +import { createSnapComponent } from '../../component'; + +/** + * The props of the {@link AccountSelector} component. + * + * @property name - The name of the account selector. This is used to identify the + * state in the form data. + * @property title - The title of the account selector. This is displayed in the UI. + * @property chainId - The chain ID of the account selector. This should be a valid CAIP-2 chain ID. + * @property selectedAddress - The default selected address of the account selector. This should be a + * valid CAIP-10 account address. + */ +export type AccountSelectorProps = { + name: string; + title: string; + chainId: CaipChainId; + selectedAddress: CaipAccountAddress; +}; + +const TYPE = 'AccountSelector'; + +/** + * An account selector component, which is used to create an account selector. + * + * This component does not accept any children. + * + * @param props - The props of the component. + * @param props.name - The name of the account selector field. This is used to identify the + * state in the form data. + * @param props.title - The title of the account selector field. This is displayed in the UI. + * @param props.chainId - The chain ID of the account selector. This should be a valid CAIP-2 chain ID. + * @param props.selectedAddress - The selected address of the account selector. This should be a + * valid CAIP-10 account address. + * @returns An account selector element. + * @example + * + * @example + * + */ +export const AccountSelector = createSnapComponent< + AccountSelectorProps, + typeof TYPE +>(TYPE); + +/** + * An account selector element. + * + * @see AccountSelector + */ +export type AccountSelectorElement = ReturnType; diff --git a/packages/snaps-sdk/src/jsx/components/form/index.ts b/packages/snaps-sdk/src/jsx/components/form/index.ts index 3dc4221376..4e47f9e9d0 100644 --- a/packages/snaps-sdk/src/jsx/components/form/index.ts +++ b/packages/snaps-sdk/src/jsx/components/form/index.ts @@ -1,3 +1,4 @@ +import type { AccountSelectorElement } from './AccountSelector'; import type { ButtonElement } from './Button'; import type { CheckboxElement } from './Checkbox'; import type { DropdownElement } from './Dropdown'; @@ -11,6 +12,7 @@ import type { RadioGroupElement } from './RadioGroup'; import type { SelectorElement } from './Selector'; import type { SelectorOptionElement } from './SelectorOption'; +export * from './AccountSelector'; export * from './Button'; export * from './Checkbox'; export * from './Dropdown'; @@ -25,6 +27,7 @@ export * from './Selector'; export * from './SelectorOption'; export type StandardFormElement = + | AccountSelectorElement | ButtonElement | CheckboxElement | FormElement diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index c58e462d1d..ff42634d56 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -32,6 +32,7 @@ import { Selector, SelectorOption, Section, + AccountSelector, } from './components'; import { AddressStruct, @@ -69,6 +70,7 @@ import { SelectorStruct, SectionStruct, NotificationComponentsStruct, + AccountSelectorStruct, } from './validation'; describe('KeyStruct', () => { @@ -956,6 +958,69 @@ describe('FileInputStruct', () => { }); }); +describe('AccountSelectorStruct', () => { + it.each([ + , + , + ])('validates an account picker element', (value) => { + expect(is(value, AccountSelectorStruct)).toBe(true); + }); + + it.each([ + 'foo', + 42, + null, + undefined, + {}, + [], + // @ts-expect-error - Invalid props. + , + // @ts-expect-error - Invalid props. + + foo + , + // @ts-expect-error - Invalid props. + , + // @ts-expect-error - Invalid props. + , + // @ts-expect-error - Invalid props. + , + // @ts-expect-error - Invalid props. + , + , + , + foo, + + foo + , + + alt + , + ])('does not validate "%p"', (value) => { + expect(is(value, AddressStruct)).toBe(false); + }); +}); + describe('SelectorStruct', () => { it.each([ diff --git a/packages/snaps-sdk/src/jsx/validation.ts b/packages/snaps-sdk/src/jsx/validation.ts index 8da02b5080..b07da65ffb 100644 --- a/packages/snaps-sdk/src/jsx/validation.ts +++ b/packages/snaps-sdk/src/jsx/validation.ts @@ -20,7 +20,9 @@ import { refine, } from '@metamask/superstruct'; import { + CaipAccountAddressStruct, CaipAccountIdStruct, + CaipChainIdStruct, hasProperty, HexChecksumAddressStruct, isPlainObject, @@ -47,6 +49,7 @@ import type { StringElement, } from './component'; import { + type AccountSelectorElement, type AddressElement, type BoldElement, type BoxElement, @@ -331,6 +334,22 @@ export const FileInputStruct: Describe = element( }, ); +/** + * A struct for the {@link AccountSelectorElement} type. + */ +export const AccountSelectorStruct: Describe = element( + 'AccountSelector', + { + name: string(), + title: string(), + chainId: CaipChainIdStruct as unknown as Struct< + Infer, + Infer + >, + selectedAddress: CaipAccountAddressStruct, + }, +); + /** * A subset of JSX elements that represent the tuple Box + Input of the Field children. */ @@ -824,6 +843,7 @@ export const JSXElementStruct: Describe = typedUnion([ SelectorStruct, SelectorOptionStruct, SectionStruct, + AccountSelectorStruct, ]); /** From 7d1501396db8093fc78ad441f6d85d22eeaeedfd Mon Sep 17 00:00:00 2001 From: Albert G <516972+alber70g@users.noreply.github.com> Date: Thu, 26 Sep 2024 18:23:05 +0200 Subject: [PATCH 4/5] fix: packageJsonStruct to allow `repository.directory` (#2754) When releasing a package from a monorepo the `repository.directory` is used to identify which package is been released. This is used for the Github build provenance. --------- Co-authored-by: Maarten Zuidhoorn --- packages/snaps-utils/src/manifest/validation.ts | 2 +- packages/snaps-utils/src/types.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/snaps-utils/src/manifest/validation.ts b/packages/snaps-utils/src/manifest/validation.ts index 3f8deef7ab..6ed252ae52 100644 --- a/packages/snaps-utils/src/manifest/validation.ts +++ b/packages/snaps-utils/src/manifest/validation.ts @@ -268,7 +268,7 @@ export const SnapManifestStruct = object({ description: size(string(), 1, 280), proposedName: size(string(), 1, 214), repository: optional( - object({ + type({ type: size(string(), 1, Infinity), url: size(string(), 1, Infinity), }), diff --git a/packages/snaps-utils/src/types.ts b/packages/snaps-utils/src/types.ts index aa2bf85cf3..abbba8b2e6 100644 --- a/packages/snaps-utils/src/types.ts +++ b/packages/snaps-utils/src/types.ts @@ -1,7 +1,6 @@ import { instance, is, - object, optional, pattern, refine, @@ -42,7 +41,7 @@ export const NpmSnapPackageJsonStruct = type({ name: NameStruct, main: optional(size(string(), 1, Infinity)), repository: optional( - object({ + type({ type: size(string(), 1, Infinity), url: size(string(), 1, Infinity), }), From 60bd1d890e2d438aa18f2a7d364b8f350a6e1cb1 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 26 Sep 2024 18:22:29 +0100 Subject: [PATCH 5/5] Convert createWindow parameters to options bag and add testid option (#2765) This PR refactors the `createWindow` function to use an options bag for its parameters, improving clarity and extensibility. Additionally, it introduces a new `testId` option, allowing customization of the `data-testid` attribute for the generated iframe. We need this change in the [MetaMask/ocap-kernel ](https://github.com/MetaMask/ocap-kernel/) package ([see issue](https://github.com/MetaMask/ocap-kernel/issues/91)), where the `createWindow` function is used, to set a different testId that doesn't conflict with the snaps iframe. This will ensure clear differentiation when using iframes across different packages. Updated and expanded the test suite to cover the presence and absence of sandbox and testId attributes. --- .../services/iframe/IframeExecutionService.ts | 5 +- .../webworker/WebWorkerExecutionService.ts | 10 ++-- .../src/proxy/ProxySnapExecutor.ts | 2 +- .../snaps-utils/src/iframe.test.browser.ts | 47 ++++++++++++++++++- packages/snaps-utils/src/iframe.ts | 25 ++++++---- 5 files changed, 73 insertions(+), 16 deletions(-) diff --git a/packages/snaps-controllers/src/services/iframe/IframeExecutionService.ts b/packages/snaps-controllers/src/services/iframe/IframeExecutionService.ts index f0c563fe8c..33fd9cbe4b 100644 --- a/packages/snaps-controllers/src/services/iframe/IframeExecutionService.ts +++ b/packages/snaps-controllers/src/services/iframe/IframeExecutionService.ts @@ -35,7 +35,10 @@ export class IframeExecutionService extends AbstractExecutionService { worker: Window; stream: BasePostMessageStream; }> { - const iframeWindow = await createWindow(this.iframeUrl.toString(), jobId); + const iframeWindow = await createWindow({ + uri: this.iframeUrl.toString(), + id: jobId, + }); const stream = new WindowPostMessageStream({ name: 'parent', diff --git a/packages/snaps-controllers/src/services/webworker/WebWorkerExecutionService.ts b/packages/snaps-controllers/src/services/webworker/WebWorkerExecutionService.ts index 7dc55a1b3d..4fb843b6f0 100644 --- a/packages/snaps-controllers/src/services/webworker/WebWorkerExecutionService.ts +++ b/packages/snaps-controllers/src/services/webworker/WebWorkerExecutionService.ts @@ -98,11 +98,11 @@ export class WebWorkerExecutionService extends AbstractExecutionService return; } - const window = await createWindow( - this.#documentUrl.href, - WORKER_POOL_ID, - false, - ); + const window = await createWindow({ + uri: this.#documentUrl.href, + id: WORKER_POOL_ID, + sandbox: false, + }); this.#runtimeStream = new WindowPostMessageStream({ name: 'parent', diff --git a/packages/snaps-execution-environments/src/proxy/ProxySnapExecutor.ts b/packages/snaps-execution-environments/src/proxy/ProxySnapExecutor.ts index f97f113b0b..5f77370912 100644 --- a/packages/snaps-execution-environments/src/proxy/ProxySnapExecutor.ts +++ b/packages/snaps-execution-environments/src/proxy/ProxySnapExecutor.ts @@ -96,7 +96,7 @@ export class ProxySnapExecutor { * @returns The executor job object. */ async #initializeJob(jobId: string): Promise { - const window = await createWindow(this.#frameUrl, jobId); + const window = await createWindow({ uri: this.#frameUrl, id: jobId }); const jobStream = new WindowPostMessageStream({ name: 'parent', target: 'child', diff --git a/packages/snaps-utils/src/iframe.test.browser.ts b/packages/snaps-utils/src/iframe.test.browser.ts index 2960ed9a6b..1b13907313 100644 --- a/packages/snaps-utils/src/iframe.test.browser.ts +++ b/packages/snaps-utils/src/iframe.test.browser.ts @@ -5,12 +5,57 @@ const IFRAME_URL = `http://localhost:4569`; const MOCK_JOB_ID = 'job-id'; describe('createWindow', () => { + afterEach(() => { + const iframe = document.getElementById(MOCK_JOB_ID); + if (iframe) { + document.body.removeChild(iframe); + } + }); + it('creates an iframe window with the provided job ID as the iframe ID', async () => { - const window = await createWindow(IFRAME_URL, MOCK_JOB_ID); + const window = await createWindow({ uri: IFRAME_URL, id: MOCK_JOB_ID }); const iframe = document.getElementById(MOCK_JOB_ID) as HTMLIFrameElement; expect(iframe).toBeDefined(); expect(iframe.contentWindow).toBe(window); expect(iframe.id).toBe(MOCK_JOB_ID); }); + + it('sets the sandbox attribute when the sandbox option is true', async () => { + await createWindow({ uri: IFRAME_URL, id: MOCK_JOB_ID, sandbox: true }); + const iframe = document.getElementById(MOCK_JOB_ID) as HTMLIFrameElement; + + expect(iframe).toBeDefined(); + expect(iframe.getAttribute('sandbox')).toBe('allow-scripts'); + }); + + it('does not set the sandbox attribute when the sandbox option is false', async () => { + await createWindow({ uri: IFRAME_URL, id: MOCK_JOB_ID, sandbox: false }); + const iframe = document.getElementById(MOCK_JOB_ID) as HTMLIFrameElement; + + expect(iframe).toBeDefined(); + expect(iframe.getAttribute('sandbox')).toBeNull(); + }); + + it('sets the data-testid attribute when provided', async () => { + const testId = 'test-id'; + + await createWindow({ + uri: IFRAME_URL, + id: MOCK_JOB_ID, + testId, + }); + const iframe = document.getElementById(MOCK_JOB_ID) as HTMLIFrameElement; + + expect(iframe).toBeDefined(); + expect(iframe.getAttribute('data-testid')).toBe(testId); + }); + + it('uses the default data-testid attribute when not provided', async () => { + await createWindow({ uri: IFRAME_URL, id: MOCK_JOB_ID }); + const iframe = document.getElementById(MOCK_JOB_ID) as HTMLIFrameElement; + + expect(iframe).toBeDefined(); + expect(iframe.getAttribute('data-testid')).toBe('snaps-iframe'); + }); }); diff --git a/packages/snaps-utils/src/iframe.ts b/packages/snaps-utils/src/iframe.ts index 38e5e8dd30..dcd3dff7aa 100644 --- a/packages/snaps-utils/src/iframe.ts +++ b/packages/snaps-utils/src/iframe.ts @@ -3,22 +3,31 @@ * forever if the iframe never loads, but the promise should be wrapped in * an initialization timeout in the SnapController. * - * @param uri - The iframe URI. - * @param id - The ID to assign to the iframe. - * @param sandbox - Whether to enable the sandbox attribute. + * + * @param options - The options for createWindow. + * @param options.uri - The iframe URI. + * @param options.id - The ID to assign to the iframe. + * @param options.sandbox - Whether to enable the sandbox attribute. + * @param options.testId - The data-testid attribute to assign to the iframe. * @returns A promise that resolves to the contentWindow of the iframe. */ -export async function createWindow( - uri: string, - id: string, +export async function createWindow({ + uri, + id, sandbox = true, -): Promise { + testId = 'snaps-iframe', +}: { + uri: string; + id: string; + sandbox?: boolean; + testId?: string; +}): Promise { return await new Promise((resolve, reject) => { const iframe = document.createElement('iframe'); // The order of operations appears to matter for everything except this // attribute. We may as well set it here. iframe.setAttribute('id', id); - iframe.setAttribute('data-testid', 'snaps-iframe'); + iframe.setAttribute('data-testid', testId); if (sandbox) { // For the sandbox property to have any effect it needs to be set before the iframe is appended.