[native] Fix SIWE panel size
ClosedPublic
Actions

Authored by tomek on Jan 10 2025, 11:22 AM.

Details

Reviewers

kamil
bartek
ashoat

Commits

rCOMM1b491537d567: [native] Fix SIWE panel size

Summary

The only purpose of onSuccessOrCancel is to stop the view from resizing after we're done with all the work. It was incorrectly called in the effect, because then the work wasn't yet completed. The intention behind calling this function there was to avoid continuing the effect when the nonce is already there - but there's no need to do that because early return already protects us against it.

https://linear.app/comm/issue/ENG-10027/siwe-wallet-connect-ui-is-shifted-when-opened-with-some-request-data

Depends on D14194

Test Plan

Tested on iOS simulator - both social proof and backup decrypt steps should have the SIWE panel displayed correctly.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Jan 10 2025, 11:22 AM

tomek added inline comments.Jan 10 2025, 11:24 AM

native/account/siwe-panel.react.js
117–119 ↗	(On Diff #46648)	We had an idea during a meeting that we should introduce a new ref to handle the early return. It looks like we don't need that because `siweNonce && siweStatement && siweIssuedAt` condition would cause an early return so there's no need to save the ref.

Harbormaster completed remote builds in B33226: Diff 46648.Jan 10 2025, 11:51 AM

tomek requested review of this revision.Jan 10 2025, 11:51 AM

Some questions

native/account/siwe-panel.react.js
99–102 ↗	(On Diff #46648)	Are you sure we don't need to call this in the `siweNonce && siweStatement && siweIssuedAt` case?
115 ↗	(On Diff #46648)	I worry that without some ref getting set, there's a possibility of some code running twice. Is there a risk of something getting called twice, and causing problems?
229–231 ↗	(On Diff #46648)	Is it concerning that this early exit might be skipped?
238 ↗	(On Diff #46648)	This one too

This revision now requires changes to proceed.Jan 10 2025, 12:01 PM

ashoat added inline comments.Jan 10 2025, 12:02 PM

native/account/siwe-panel.react.js
106 ↗	(On Diff #46648)	Does this condition only trigger when `messageType` is `MSG_BACKUP` or `MSG_BACKUP_RESTORE`? If so, it might be helpful to add an `invariant`, or at least a code comment

tomek requested review of this revision.Jan 13 2025, 5:53 AM

tomek added inline comments.

native/account/siwe-panel.react.js
99–102 ↗	(On Diff #46648)	Initially, we don't want to call this. When this component is mounted, an effect is called from which this function is called. This doesn't make sense, because there isn't a timeout that needs to be cleared. We should call the `onSuccessOrCancel` callback later. For all the message types, ultimately, `siwe_success` or `siwe_closed` event is sent, where we call `onSuccessOrCancel`.
106 ↗	(On Diff #46648)	I don't think we should consider the `messageType` here. What matters is whether a nonce was provided to us, or we need to get one. If it was provided, then we should early return from this effect. And this should happen regardless of the message type.
115 ↗	(On Diff #46648)	In the next line, we have a `return` statement, which means that we will finish this effect regardless of the presence of a ref. So I don't think there's a risk of calling something twice and causing problems. There are some things that are already called multiple times - the whole body of this `if`.
229–231 ↗	(On Diff #46648)	Quite the opposite - before `siwe_success` or `siwe_closed` event is sent, we should not early exit here. Only after we're done with the work, we should start skipping this event.