Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Account for a custom nonce of zero #8883

Merged
merged 2 commits into from
Jul 2, 2020
Merged

Conversation

tmashuang
Copy link
Contributor

Fixes #8882

Seems that 0 isn't valid in || logic operator.

@tmashuang tmashuang requested a review from a team as a code owner July 2, 2020 09:31
@metamaskbot
Copy link
Collaborator

Builds ready [6429004]
Page Load Metrics (1296 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded11161654129315374
load11171657129615374
domInteractive11161654129315374

Copy link
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.

We still use falsey checks on customNonceValue in a number of places, e.g. in confirm-transaction-base.container.js, which might be the only place this gets set in txMeta in practice? 🤔 So it still seems like this shouldn't work, unless I'm missing something.

@tmashuang
Copy link
Contributor Author

tmashuang commented Jul 2, 2020

I think in those instances it is set to a string.

Edit: I don't see where those instances where we type check customNonceValue .

@Gudahtt
Copy link
Member

Gudahtt commented Jul 2, 2020

Ah I see - makes sense. I'll take another look.

Copy link
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!

@tmashuang tmashuang merged commit 259d198 into develop Jul 2, 2020
@tmashuang tmashuang deleted the customNonce-account-for-zero branch July 2, 2020 19:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom nonce doesn't work for value 0
3 participants