add MetaMask Template Renderer by brad-decker · Pull Request #10307 · MetaMask/metamask-extension

brad-decker · 2021-01-28T19:45:29Z

Rationale

I have spoken before of a templating system that simply maps to React children/props. I hope that some version of this can be used by Snaps to customize the UI of MetaMask. I don't expect that the API proposed here is ready for that endeavor, however, this gives us a real-world tool to start playing around with what would be acceptable as an API for templating. It will also be insanely useful for quickly scaffolding UI.

API Design

Everything centers around Sections. Each Section has this shape:

interface Section {
  children: Section[] | string;
  props: Record<string, any>;
  element: keyof SafeComponentList;
}

The MetaMaskTemplateRenderer accepts a sections prop that is either an array of Sections, an array of strings, an array of mixed strings and Sections, or a string. The renderer will first check if sections is a string and simply return it. Otherwise, it reduces over the array of Sections. In the event of a string, the string is pushed into the accumulator. In the event of a Section, the element is looked up from the safe-component-list and becomes the Element so it can be rendered as a react component. The props provided in the Section are spread into the component. If the Section has children, we again invoke MetaMaskTemplateRenderer passing children as sections. In this way, the renderer recursively traverses the object and renders all elements.

I think that this API will evolve to a point where our design-system "Molecules" (that is, fully designed sections with clear UX) are the only "safe" components. So instead of rendering a Box, with two Typography, etc, we render fully formed UI pieces. A good example of what this might look like is the TruncatedDefinitionList which just pieces together other UI components and adds functionality.

Safe guards

An accompanying safe-component-list acts as an attenuation, preventing all possible HTML tags from being rendered, and allowing us to explicitly hook up our custom components.
If an element doesn't exist in the safe-component-list it is skipped and its children are not rendered. A warning is issued in the console. This could be upgraded to throw an Error instead.

Examples

Quick scaffolding of UI


[
  {
    "element": "Box",
    "props": {
      "margin": 4,
      "padding": 8,
      "borderColor": "primary-1",
      "borderWidth": 2
    },
    "children": [
      {
        "element": "Typography",
        "children": "A Test String",
        "props": {
          "color": "ui-3",
          "variant": "h2"
        }
      },
      {
        "element": "Typography",
        "children": "Some more text as a paragraph",
        "props": {
          "color": "ui-4",
          "variant": "paragraph"
        }
      },
      {
        "element": "TruncatedDefinitionList",
        "props": {
          "dictionary": {
            "term": "a word or phrase used to describe a thing or to express a concept, especially in a particular kind of language or branch of study.",
            "definition": "a statement of the exact meaning of a word, especially in a dictionary.",
            "dl": "HTML tag denoting a definition list",
            "dt": "HTML tag denoting a definition list term",
            "dd": "HTML tag denoting a definition list definition"
          },
          "prefaceKeys": [
            "term",
            "definition"
          ]
        }
      },
      {
        "element": "Box",
        "children": [
          {
            "element": "Button",
            "children": "Cancel",
            "props": {
              "rounded": true,
              "type": "outlined",
              "style": {
                "width": "45%"
              }
            }
          },
          {
            "element": "Button",
            "children": "OK",
            "props": {
              "rounded": true,
              "type": "primary",
              "style": {
                "width": "45%"
              }
            }
          }
        ],
        "props": {
          "justifyContent": "space-between",
          "padding": [
            0,
            4
          ]
        }
      }
    ]
  }
]

With an unsafe component


[
  {
    "element": "Box",
    "props": {
      "margin": 4,
      "padding": 8,
      "borderColor": "primary-1",
      "borderWidth": 2
    },
    "children": [
      {
        "element": "Typography",
        "children": "A Test String",
        "props": {
          "color": "ui-3",
          "variant": "h2"
        }
      },
      {
        "element": "Typography",
        "children": "Some more text as a paragraph",
        "props": {
          "color": "ui-4",
          "variant": "paragraph"
        }
      },
      {
        "element": "TruncatedDefinitionList",
        "props": {
          "dictionary": {
            "term": "a word or phrase used to describe a thing or to express a concept, especially in a particular kind of language or branch of study.",
            "definition": "a statement of the exact meaning of a word, especially in a dictionary.",
            "dl": "HTML tag denoting a definition list",
            "dt": "HTML tag denoting a definition list term",
            "dd": "HTML tag denoting a definition list definition"
          },
          "prefaceKeys": [
            "term",
            "definition"
          ]
        }
      },
      {
        "element": "Box",
        "children": [
          {
            "element": "Button",
            "children": "Cancel",
            "props": {
              "rounded": true,
              "type": "outlined",
              "style": {
                "width": "45%"
              }
            }
          },
          {
            "element": "Button",
            "children": "OK",
            "props": {
              "rounded": true,
              "type": "primary",
              "style": {
                "width": "45%"
              }
            }
          }
        ],
        "props": {
          "justifyContent": "space-between",
          "padding": [
            0,
            4
          ]
        }
      }
    ]
  },
  {
    element: 'Unsafe',
    children:
      'I should be displayed, but I wont be due to unsafe component',
  },
]

metamaskbot · 2021-01-28T20:14:16Z

Builds ready [59223b9]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (863 ± 46 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	114	82	12	6
		domContentLoaded	555	993	861	96	46
		load	556	993	863	96	46
		domInteractive	554	992	861	96	46

rekmarks

I won't finish reviewing this until later, but I have to say, metamask-template-renderer.js is a thing of beauty.

rekmarks

This PR owns. One nit and one question/comment. No blockers.

rekmarks · 2021-02-01T04:35:11Z

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js

+  }
+  return (
+    <>
+      {sections.reduce((acc, child, index) => {


nit: We should rename acc to allChildren (or something better) for great specificity while avoiding a collision with the children constant assigned on L21.

good call :D

rekmarks · 2021-02-01T04:45:18Z

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js

+  sections: ValidChildren,
+}
+
+export default memo(MetaMaskTemplateRenderer)


By my reading of the docs for React.memo, I suspect we'll need to pass a custom comparison function as the second parameter to memo to get a significant performance boost:

By default it will only shallowly compare complex objects in the props object.

If I'm right, I don't want to hold up this PR on that point, and would rather see the call to memo removed than cause delays. We can always optimize in a follow-up.

Question: The recursive calls are not memoized the way this is written, right?

I think this could be an easy win.

More background

The current implementations of MetaMaskTemplateRenderer handle memoization themselves, only passing referentially equal arrays to the sections prop.

For example, the new confirmation UI page has to generate template values based on the current confirmation. Because calling the template generator function is expensive, it is memoized and therefore the value will be referentially equal as long as the confirmation doesn't change.

Another scenario we might see is where the template is just a const and doesn't need to change. In which case, we don't need the isEqual check on the memo call.

Another case is we might be receiving a template payload from another provider, be it our own background or a third party in the future. In those cases, we'll be deserializing the template in some form or fashion and will want to handle memory allocation specifically.

lodash.isEqual

The very first thing this function does is check referential equality, and then it ejects early if they are equal. So even though the usages of the component might be passing referentially equal objects, having isEqual as the comparator function will give easy optimization for future usages that don't care so much about the values they are passing in. It also would mean that if the memoized values are updated but the resulting object is the same, this would also prevent a re-render.

Recursive calls

They should bypass the memo method as written, which was done purposefully to allow the root node of the tree to be the sole manager of re-rendering.

metamaskbot · 2021-02-01T19:11:23Z

Builds ready [368c57e]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (650 ± 120 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	353	1247	644	244	117
		load	354	1248	650	249	120
		domInteractive	353	1246	643	244	117

rekmarks

🚀 🌔

Gudahtt · 2021-02-01T21:43:32Z

An accompanying safe-component-list acts as an attenuation, preventing all possible HTML tags from being rendered, and allowing us to explicitly hook up our custom components.

Could you clarify what the goals of this list are? In what respect are these components "safe" (e.g. is it just to prevent injecting scripts)?

If an element doesn't exist in the safe-component-list it is skipped and its children are not rendered. A warning is issued in the console. This could be upgraded to throw an Error instead.

It sounds like an error would be better here 🤔 This might lead to components being accidentally omitted. It sounds like a developer mistake, which generally we should throw for.

brad-decker · 2021-02-01T21:53:14Z

Could you clarify what the goals of this list are? In what respect are these components "safe" (e.g. is it just to prevent injecting scripts)?

The safe-component-list is perhaps an aspirational name. It's primary purpose is to map 'element' to actual components that can be rendered, rather than allowing every HTML tag and component in the codebase. I would like to refine this in the future so that props for each component in the list can be attenuated, banned, or automatically filled.

e.g, an a tag can be considered "safe" as long as it has no onClick handler, the hostname of the href is in a predefined list of domains that we generally regard as safe, and the target attribute is set to __blank. This is an example, we might want to refine the rules a bit here.

The main thing it is used for right now is just to map element names to Components. This, I believe, is sufficient for us while testing this out and getting a feel for it. If ever this thing evolves to be used in snaps it'll need much more rigorous controls for validation and safety checks.

It sounds like an error would be better here 🤔 This might lead to components being accidentally omitted. It sounds like a developer mistake, which generally we should throw for.

I'm ambivalent about this, so I'll yield to your suggestion.

rekmarks · 2021-02-01T22:07:13Z

I support failing hard and fast by throwing re: the renderer being passed disallowed components.

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js

Gudahtt · 2021-02-01T22:18:26Z

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js

+            ) : null
+          if (Element) {
+            allChildren.push(
+              <Element key={`${element}${index + 0}`} {...props}>


Should we have a default key? 🤔 This should probably be provided to the template renderer, to ensure that distinct elements are treated as distinct by React. To do otherwise risks breaking in situations where similar components get moved around, and that moving has a side-effect (e.g. CSS transitions). Effectively we're silencing the React warning here.

I get why you did it though. Requiring a key for every entry is demanding.

We could eliminate the key requirement in the single-element case by returning it directly, rather than in an array. It'd require another condition in the renderer for cases where sections is a single element or an array with one entry.

I added object-hash, which I was planning on using as part of the transaction -> activity refactor and grouping by intent. This allows me to generate a key based on the shape of sections, the child we are rendering, and the current index. I strongly prefer to not make key something that is required in the object entries if we can avoid it.

This again serves only to silence the warning, and defeats the purpose of the key: to distinguish the identity of components as props change. This is functionally identical to what this was before, isn't it?

Actually, I'd stake some eth on this being worse than just using the element name and the index. What I did will actually change the key anytime props change anywhere in the tree because the props were included in the hash. I'm not sure what I was thinking, keys are meant to provide a stable identifier for the element. It should be the same even when props change.

I still really feel like adding a key to every entry to be tedious. however,

the list and items are static–they are not computed and do not change;

the items in the list have no ids;

the list is never reordered or filtered.

When all of them are met, you may safely use the index as a key.
Source: index-as-akey-is-an-anti-pattern

the list and items are static-they are not computed and do not change
with the current implementation, the sections object only changes when the confirmation itself changes. In these cases, we're dealing with, potentially, an entirely different tree of components. They are computed, so it doesn't meet the full requirement, but we aren't dealing with a list of items in a <li>.

the items in the list have no ids
I don't see a way of generating a stable id for these objects, but we could ask the dev to id the section manually.

the list is never reordered or filtered
We can't make any strong assurances on this. In the future, we may want to render a sortable list component with the template language, which would place us in violation of this rule as well.

I'm going to work on implementing your suggestion, @Gudahtt

Maybe we could add an optional prop to the renderer that declares something as static, meeting all three of those requirements? Then we could auto-generate keys in the manner you did previously.

As long as we don't auto-generate a key in the default case, we should be OK.

I updated this to allow an object as valid children, indicating a single node.

To recap possible values for sections and sections.children are now:

a string

a object with the shape of sections

an array of objects with the shape of sections

when dealing with an object or string, we don't require key. When dealing with an array we do.

check the latest commit to see how the story/code changed. I think this is the safest approach. Optimizations around key generation can come later if it becomes burdensome.

@Gudahtt i just rewrote that commit a bit, its now much simpler

One more optimization to reduce recursion calls where appropriate.

highlighting the key being attached to various places.

metamaskbot · 2021-02-02T13:41:08Z

Builds ready [561db1b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (589 ± 28 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	79	60	8	4
		domContentLoaded	414	707	588	58	28
		load	415	708	589	58	28
		domInteractive	414	706	588	58	28

metamaskbot · 2021-02-02T16:47:37Z

Builds ready [c432540]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (585 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	49	93	61	10	5
		domContentLoaded	356	752	584	69	33
		load	357	752	585	69	33
		domInteractive	356	751	583	69	33

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js

Gudahtt

LGTM!

metamaskbot · 2021-02-02T17:25:43Z

Builds ready [cd94e27]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (605 ± 41 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	47	84	62	11	5
		domContentLoaded	342	716	604	85	41
		load	343	717	605	85	41
		domInteractive	341	716	603	85	41

brad-decker requested a review from a team as a code owner January 28, 2021 19:45

brad-decker requested a review from NiranjanaBinoy January 28, 2021 19:45

add MetaMask Template Renderer

59223b9

brad-decker force-pushed the add-metamask-template-renderer branch from 660c0d8 to 59223b9 Compare January 28, 2021 19:55

brad-decker marked this pull request as draft January 28, 2021 19:55

brad-decker removed the request for review from NiranjanaBinoy January 28, 2021 19:55

brad-decker marked this pull request as ready for review January 28, 2021 20:15

brad-decker requested review from Gudahtt and rekmarks January 28, 2021 20:16

rekmarks reviewed Jan 28, 2021

View reviewed changes

rekmarks reviewed Feb 1, 2021

View reviewed changes

rekmarks previously approved these changes Feb 1, 2021

View reviewed changes

Gudahtt reviewed Feb 1, 2021

View reviewed changes

brad-decker dismissed rekmarks’s stale review via 561db1b February 1, 2021 22:56

add areEqual fn and change acc var name

e168b68

brad-decker force-pushed the add-metamask-template-renderer branch from 561db1b to e168b68 Compare February 2, 2021 15:15

brad-decker force-pushed the add-metamask-template-renderer branch from c432540 to c85a06b Compare February 2, 2021 16:48

use key

cd94e27

brad-decker force-pushed the add-metamask-template-renderer branch from c85a06b to cd94e27 Compare February 2, 2021 17:03

Gudahtt reviewed Feb 2, 2021

View reviewed changes

ui/app/components/app/metamask-template-renderer/metamask-template-renderer.js Show resolved Hide resolved

Gudahtt approved these changes Feb 2, 2021

View reviewed changes

brad-decker merged commit 96933b3 into develop Feb 2, 2021

brad-decker deleted the add-metamask-template-renderer branch February 2, 2021 18:14

github-actions bot locked and limited conversation to collaborators Feb 2, 2021

Conversation

brad-decker commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

API Design

Safe guards

Examples

Uh oh!

metamaskbot commented Jan 28, 2021

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

More background

lodash.isEqual

Recursive calls

Uh oh!

metamaskbot commented Feb 1, 2021

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt commented Feb 1, 2021

Uh oh!

brad-decker commented Feb 1, 2021

Uh oh!

rekmarks commented Feb 1, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad-decker Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gudahtt Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 2, 2021

Uh oh!

metamaskbot commented Feb 2, 2021

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

brad-decker commented Jan 28, 2021 •

edited

Loading

brad-decker Feb 2, 2021 •

edited

Loading

Gudahtt Feb 2, 2021 •

edited

Loading