fix: Fix Trezor signing and connecting#26882
Conversation
1daedb7 to
eb0766e
Compare
There was a problem hiding this comment.
This is where the underscore was missing
There was a problem hiding this comment.
The same settings object is referenced here, so we'd want to use the same modifications. This was present in the original patch
There was a problem hiding this comment.
This second call to events_2.POPUP.INIT was not present in v9.2.2, but it is present here in v9.3.0
See:
9.2.2: https://npmfs.com/package/@trezor/connect-web/9.2.2/lib/popup/index.js
9.3.0: https://npmfs.com/package/@trezor/connect-web/9.3.0/lib/popup/index.js
Trezor signing and connecting was broken in a recent change that included an update to the `@trezor/connect-web` library (in #26143). The patch had to be rewritten as part of that update, but in this rewrite an underscore was missed in referencing a variable (the patch used `this.settings` rather than `this._settings`). Additionally, two more changes have been applied to ensure the modified settings are used everywhere. One was present in the original patch but missing from the patch in #26143. The other was not present in the original patch (#23763) because the affected line was added in the library update, but it seems equally necessary. Fixes #26875
eb0766e to
11d653e
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26882 +/- ##
========================================
Coverage 70.15% 70.15%
========================================
Files 1425 1425
Lines 49600 49600
Branches 13877 13877
========================================
Hits 34795 34795
Misses 14805 14805 ☔ View full report in Codecov by Sentry. |
Builds ready [11d653e]
Page Load Metrics (1634 ± 84 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
danjm
left a comment
There was a problem hiding this comment.
Approved. I have reviewed all code and manually tested both connecting trezor and signing+sending a transaction with trezor
mikesposito
left a comment
There was a problem hiding this comment.
Looks good, and it works!
There was a problem hiding this comment.
Are we not using modifiedSettings in this branch both here and in the original patch because we want a longer request timeout for handshake events?
@@ -240,7 +241,7 @@ class PopupManager extends events_1.default {
else if (message.type === events_2.POPUP.CORE_LOADED) {
+ var modifiedSettings = Object.assign({}, this.settings, { env: 'webextension' });
this.channel.postMessage({
type: events_2.POPUP.HANDSHAKE,
- payload: { settings: this.settings },
+ payload: { settings: modifiedSettings },
});
(_a = this.handshakePromise) === null || _a === void 0 ? void 0 : _a.resolve();
}There was a problem hiding this comment.
Great question. Maybe we should be using it here as well
There was a problem hiding this comment.
I'll merge this for now, but this is worth following up on
Description
Trezor signing and connecting was broken in a recent change that included an update to the
@trezor/connect-weblibrary (in #26143). The patch had to be rewritten as part of that update, but in this rewrite an underscore was missed in referencing a variable (the patch usedthis.settingsrather thanthis._settings).Additionally, two more changes have been applied to ensure the modified settings are used everywhere. One was present in the original patch but missing from the patch in #26143. The other was not present in the original patch (#23763) because the affected line was added in the library update, but it seems equally necessary.
Related issues
Fixes #26875
Manual testing steps
Follow reproduction steps in #26875
Screenshots/Recordings
Before
See https://www.loom.com/share/3e1f3d2cc57247a5b02e0ff8d61babb8?sid=1b17e423-16cf-4260-a1ed-674441e25b3a
After:
Screen.Recording.2024-09-03.at.21.55.10.mov
(I stopped partway through connecting, I was already past the point of it crashing by then).
Pre-merge author checklist
Pre-merge reviewer checklist