Refactor: Move all components to separate files #379

Montoya · 2024-12-17T04:37:28Z

This refactors everything as independent modules with shared global context. We also attempted to

make these modules as self-contained as possible.
move constants and functions into separate files.
use shared constants and objects for sharing state.
add events and listeners where appropriate for handling state changes.

This refactor does not:

Change the connect / disconnect / provider logic very much
Rewrite with React

But it's progress

* chore: refactor signature types to different components * fix: injection order of components

And committing a naming crime, but we will allow it

This introduces a new parameters on `globalContext` called `_connected` with a getter and setter. When set to a new value, the setter dispatches a custom event `globalConnectionChange`. Each module with a button that needs to be enabled when MetaMask is connected thus has an event listener for that event. Now, those buttons are enabled via the listener. This gets us part way to relying on listeners for all state changes.

Also fix a few small errors from previous commits

* chore: Split transactions into separate components * chore: add index.js to export all modules * chore: add index.js to export all modules

src/components/transactions/erc20.js

Unfortunately most of the connection logic is still in index.js and is hard to untangle

src/components/transactions/erc20.js

pedronfigueiredo · 2024-12-20T17:57:32Z

src/components/signatures/signTypedDataV3-sign.js

+export function signTypedDataV3Component(parentContainer) {
+  parentContainer.insertAdjacentHTML(
+    'beforeend',
+    `<div class="col-xl-4 col-lg-6 col-md-12 col-sm-12 col-12 d-flex align-items-stretch">


This is great! Would it make sense to keep a separate HTML file for each component so that it has syntax highlighting, and import the markup here?

Yes. There are other ways to do it too, like importing a JSX parser so you can use JSX fragments instead of strings.

(Or just use a plugin, or just bite the bullet and port to React :) )

pnarayanaswamy · 2025-01-06T10:48:46Z

Ran all e2e on chrome and firefox successfully with a patch version from local: MetaMask/metamask-extension#29433

seaona

hi there! Nice work 🔥
I'm in the middle of the testing, will update this comment with my findings.

So far, I found that whenever I am using the Wallet Connect functionalty several buttons remain disabled (deploy contracts, signatures..) see video with the comparision of current prod and this test dapp version below:

wallet-connect-btn-disabled.mp4

[Update] The rest seems to work fine on my end!

seaona · 2025-01-17T07:59:30Z

src/components/signatures/signTypedData-variants.js

+                </p>
+              </div>
+            </div>
+          </div>`,


Nice new section! Suggestion: could we add a Verify section too, so we are sure the wallet works correctly with those signature values?
No need to be in this PR, but would be nice to have this, at some point

Montoya · 2025-01-17T14:36:46Z

hi there! Nice work 🔥 I'm in the middle of the testing, will update this comment with my findings.

So far, I found that whenever I am using the Wallet Connect functionalty several buttons remain disabled (deploy contracts, signatures..) see video with the comparision of current prod and this test dapp version below:

wallet-connect-btn-disabled.mp4
[Update] The rest seems to work fine on my end!

Hmmm @pnarayanaswamy we will have to figure out where this might have gone wrong... maybe just missed some elements in the events?

Montoya · 2025-01-20T15:02:25Z

hi there! Nice work 🔥 I'm in the middle of the testing, will update this comment with my findings.

So far, I found that whenever I am using the Wallet Connect functionalty several buttons remain disabled (deploy contracts, signatures..) see video with the comparision of current prod and this test dapp version below:

wallet-connect-btn-disabled.mp4
[Update] The rest seems to work fine on my end!

Please try again, just merged a fix by @pnarayanaswamy

seaona

thanks, I see the issue fixed now!!

wc-dapp.mp4

seaona · 2025-01-21T14:06:13Z

src/components/transactions/eip747.js

nit: this functionality seems to be incorrectly placed under transactions

seaona · 2025-01-21T14:12:20Z

src/sample-networks.js

nit: this file could be consolidated with the network data we already have here

https://github.com/MetaMask/test-dapp/blob/main/src/onchain-sample-contracts.js#L2

seaona · 2025-01-21T14:16:02Z

src/components/ppom/transactions.js

+            Malicious Permit
+          </button>
+          <button
+            class="btn btn-primary btn-lg btn-block mb-3"
+            id="maliciousTradeOrder"
+            disabled
+          >
+            Malicious Trade Order
+          </button>
+          <button
+            class="btn btn-primary btn-lg btn-block mb-3"
+            id="maliciousSeaport"
+            disabled
+          >
+            Malicious Seaport
+          </button>


nit: this section is placed under ppom/transactions but those are actually signatures

Malicious Permit

Malicious Trade Order

Malicious Seaport

Yes, this is how the test-dapp was originally organized. We could rename transactions.js to something else.

seaona · 2025-01-21T14:17:42Z

src/components/signatures/malformed-transactions.js

the following content is related to transactions, but it's placed under signatures folder

Yes, the way the cards are grouped is based on the name of the section. It's not ideal.

One solution could be to move this card to the transactions section, but I did not want to make any IA changes

mm yeah so I think something could be improved there, as at the moment the current folder naming doesn't completely match the classification 🤔

mm if I imagine if I want to add a new section, let's say Solana tx's and signatures, where will they go?
I think the natural way might be to create a solana folder, and inside, to have a signature component and then a tx component.

So I think we might want to follow the same approach here. We could have a folder for Malformed Interactions and inside we can distinguish between signatures and tx. Same goes for Blockaid and so on.

-- Malformed Interactions
|
|--- Signatures
|--- Transactions

-- Malicious Interactions
|
|--- Signatures
|--- Transactions

-- Solana
|
|--- Signatures
|--- Transactions

What do you think?

seaona · 2025-01-21T14:20:14Z

The functionality seems to be preserved so I've approved the PR, but recently added a couple of nits regarding the re-org, non-blocking but might be worth taking a look now or in a future PR

Montoya · 2025-01-21T14:36:30Z

@seaona I think overall the IA nits should be for the developers who made these cards. I don't want to mess with how the cards are grouped and sorted, my intention was to keep the IA the same.

This is the release candidate PR for `v9.0.0` ### Added - Add SignTypedData primaryType variants: Blur - Order, PermitBatch, PermitSingle, Seaport - BulkOrder ([#376](#376)) ### Changed - Move all components to separate files, use shared global state and events for updating cards based on connect / disconnect / deploy contract ([#379](#379))

* Release Candidate v9.0.0 This is the release candidate PR for `v9.0.0` ### Added - Add SignTypedData primaryType variants: Blur - Order, PermitBatch, PermitSingle, Seaport - BulkOrder ([#376](#376)) ### Changed - Move all components to separate files, use shared global state and events for updating cards based on connect / disconnect / deploy contract ([#379](#379)) * Put Unreleased lines back in CHANGELOG

seaona · 2025-01-22T07:54:48Z

hi there 👋 thanks for the reply!! I made a couple of the cards mentioned above so I'm happy to add the changes if they make sense to you!

Montoya and others added 13 commits December 16, 2024 23:35

Move ENS Resolution to separate file

f117233

lint fixes

137313b

Move send form to separate file

fc075e7

chore: refactor signature types to different components (#380)

7b2cba1

* chore: refactor signature types to different components * fix: injection order of components

Simplify loading a batch of modules

9a87345

Use parentContainer pattern across all components

99869ee

Migrate malformed transactions form

623a8a1

And committing a naming crime, but we will allow it

Move encrypt-decrypt to separate module

45452f6

Migrate encrypt-decrypt and all PPOMs

65af96a

Also fix a few small errors from previous commits

Fix imports

0a83d5e

fix: permit message verification (#382)

089c21f

chore: Split transactions into separate components (#383)

527956e

* chore: Split transactions into separate components * chore: add index.js to export all modules * chore: add index.js to export all modules

github-advanced-security bot found potential problems Dec 18, 2024

View reviewed changes

src/components/transactions/erc20.js Fixed Show fixed Hide fixed

Montoya added 3 commits December 18, 2024 17:46

Move interactions section to module files

05f1b21

Move connections to separate files

3cd4ab6

Unfortunately most of the connection logic is still in index.js and is hard to untangle

Wrap an error message in template string

2d31684

github-advanced-security bot found potential problems Dec 19, 2024

View reviewed changes

src/components/transactions/erc20.js Fixed Show fixed Hide fixed

Montoya added 12 commits December 18, 2024 21:54

Migrate type onchange handler to module

c835453

Move sendDeepLink href set to module

70df8ab

set deep links in erc20 component code

9b9cc05

Fix glaring typo

37170e0

Programmatic sections

87f3d0c

lint fixes

7ea98e8

Move more events and utils

8d24355

Lint fix and remove array

5833f15

Decentralize clearDisplayElements

c5df359

No more allConnectedButtons

11dd3f1

Change innerHTML to innerText

fc91395

No more initializeFormElements

c0460d8

Montoya marked this pull request as ready for review December 20, 2024 04:58

Montoya requested a review from pnarayanaswamy December 20, 2024 04:58

pnarayanaswamy requested a review from seaona December 20, 2024 12:20

Montoya changed the title ~~WIP: Refactor: Move all components to separate files~~ Refactor: Move all components to separate files Dec 20, 2024

Remove unneeded lint disable line

6ea8a5d

pedronfigueiredo reviewed Dec 20, 2024

View reviewed changes

pnarayanaswamy previously approved these changes Jan 6, 2025

View reviewed changes

pedronfigueiredo previously approved these changes Jan 9, 2025

View reviewed changes

seaona requested changes Jan 16, 2025

View reviewed changes

seaona reviewed Jan 17, 2025

View reviewed changes

fix: enable defaults on wallet connect (#384)

b8b2cbb

Montoya dismissed stale reviews from pedronfigueiredo and pnarayanaswamy via b8b2cbb January 20, 2025 15:01

seaona approved these changes Jan 21, 2025

View reviewed changes

seaona reviewed Jan 21, 2025

View reviewed changes

src/components/transactions/eip747.js

Copy link

Contributor

seaona Jan 21, 2025

Choose a reason for hiding this comment

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

nit: this functionality seems to be incorrectly placed under transactions

seaona reviewed Jan 21, 2025

View reviewed changes

Montoya merged commit dac0701 into main Jan 21, 2025
7 checks passed

Montoya deleted the modular branch January 21, 2025 14:21

Montoya mentioned this pull request Jan 21, 2025

Release Candidate v9.0.0 #385

Merged

MetaMask deleted a comment Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Move all components to separate files #379

Refactor: Move all components to separate files #379

Montoya commented Dec 17, 2024 •

edited

Loading

pedronfigueiredo Dec 20, 2024

Montoya Dec 20, 2024

Montoya Dec 20, 2024 •

edited

Loading

pnarayanaswamy commented Jan 6, 2025 •

edited

Loading

seaona left a comment •

edited

Loading

seaona Jan 17, 2025

Montoya commented Jan 17, 2025

Montoya commented Jan 20, 2025

seaona left a comment

seaona Jan 21, 2025

seaona Jan 21, 2025 •

edited

Loading

seaona Jan 21, 2025 •

edited

Loading

Montoya Jan 21, 2025

seaona Jan 21, 2025

Montoya Jan 21, 2025

Montoya Jan 21, 2025

seaona Jan 22, 2025

seaona commented Jan 21, 2025

Montoya commented Jan 21, 2025

seaona commented Jan 22, 2025

Refactor: Move all components to separate files #379

Refactor: Move all components to separate files #379

Conversation

Montoya commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Montoya Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

pnarayanaswamy commented Jan 6, 2025 • edited Loading

seaona left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Montoya commented Jan 17, 2025

Montoya commented Jan 20, 2025

seaona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaona Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

seaona Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaona commented Jan 21, 2025

Montoya commented Jan 21, 2025

seaona commented Jan 22, 2025

Montoya commented Dec 17, 2024 •

edited

Loading

Montoya Dec 20, 2024 •

edited

Loading

pnarayanaswamy commented Jan 6, 2025 •

edited

Loading

seaona left a comment •

edited

Loading

seaona Jan 21, 2025 •

edited

Loading

seaona Jan 21, 2025 •

edited

Loading