Skip to content

handle undefined network#10560

Merged
brad-decker merged 1 commit intodevelopfrom
fix-ens-input
Mar 3, 2021
Merged

handle undefined network#10560
brad-decker merged 1 commit intodevelopfrom
fix-ens-input

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Mar 2, 2021 •
edited by Gudahtt
Loading

Fixes: when switching networks the chainId is temporarily undefined, resulting in trying to construct a new instance of ENS without a network. This fix will prevent constructing ENS when the network isn't supported yet for ENS lookup. It will set this.ens to null when switching to an unsupported network.

Fixes #10559

@brad-decker brad-decker marked this pull request as ready for review March 2, 2021 20:53
@brad-decker brad-decker requested a review from a team as a code owner March 2, 2021 20:53
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 2, 2021

Builds ready [448491e]
Page Load Metrics (714 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint459564136
domContentLoaded42398271212158
load42498471412158
domInteractive42298271212158

@tmashuang
Copy link
Copy Markdown
Contributor

tmashuang commented Mar 2, 2021

LGTM.

Gudahtt
Gudahtt previously approved these changes Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Though I think I've just discovered another related bug: this.checkName doesn't ever get initialized if the user switches from an unsupported network to a supported network while on this screen

@brad-decker
Copy link
Copy Markdown
Contributor Author

brad-decker commented Mar 2, 2021

@Gudahtt scratches head this.checkName isn't even used... I guess we decided not to denounce lookupEnsName after all? Should I:
A) Remove it
B) use it

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 2, 2021

Builds ready [01bf74b]
Page Load Metrics (569 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44685573
domContentLoaded3476795678842
load3486795698842
domInteractive3476785678842

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Mar 2, 2021

Huh. I started looking into what lookupEnsName does, and I suspect this is susceptible to race conditions. It does async stuff without considering for the fact that they might complete out of order.

I think the debounce makes sense? But I'm a little concerned about using it without fixing this race condition, on the off chance that switching to use the debounce changes the timing in a way that exposes this issue further.

Gudahtt
Gudahtt approved these changes Mar 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Oh, oops, I meant my earlier LGTM comment to be an approval 😅 Edit: Oh I see, it was dismissed, I misread.

The other stuff we found can be handled in a later PR.

@brad-decker brad-decker merged commit eef92d0 into develop Mar 3, 2021
@brad-decker brad-decker deleted the fix-ens-input branch March 3, 2021 00:20
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@NiranjanaBinoy NiranjanaBinoy Awaiting requested review from NiranjanaBinoy

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error: The EthJsENS Constructor requires a network or registry address.

4 participants

@brad-decker @metamaskbot @tmashuang @Gudahtt