Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Replace undefined selectedAddress with null #7161

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Sep 12, 2019

The runtime.Port.postMessage API will drop keys with a value of undefined on Chrome, but not on Firefox. This was a problem for the publicConfig stream, which passed the key selectedAddress with the value of undefined to communicate to dapps that the user had logged
out.

Instead a null is now passed for selectedAddress upon logout, which is correctly sent by the runtime.Port.postMessage API on both Chrome
and Firefox.

Relevant Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1003439

closes #7101
closes #7109

The `runtime.Port.postMessage` API will drop keys with a value of
`undefined` on Chrome, but not on Firefox. This was a problem for the
`publicConfig` stream, which passed the key `selectedAddress` with the
value of `undefined` to communicate to dapps that the user had logged
out.

Instead a `null` is now passed for `selectedAddress` upon logout, which
is correctly sent by the `runtime.Port.postMessage` API on both Chrome
and Firefox.

closes #7101
closes #7109
@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 12, 2019

This will result in a public-facing change to the accountsChanged event emitted by metamask-inpage-provider, so we may want to hold off on merging this until we make an accompanying change there to re-map this back to the earlier format.

Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

Safe and easy.

@metamaskbot
Copy link
Collaborator

Builds ready [3870cfe]

@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 12, 2019

We've decided that both [undefined] and [null] are not great responses for the accountsChanged event, and arguably don't confirm to EIP-1193 (which states that accountsChanged should emit an array of strings). So instead we're making a different public-facing change to emit an empty array in the case where the user is logged out.
MetaMask/providers#8

Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

Marking the request for an empty array.

The v3.0.0 update includes a change to the `accountsChanged` event. The
event will now emit an empty array instead of an array with `undefined`
or `null`.

The previous behavior was to emit `[undefined]`. The previous commit
would have changed that to `[null]` anyway, so we figured if we're
going to make a public-facing change to the event anyway we should
change it to be correct. `[undefined]` was never intended, and it
technically violates EIP-1193, which states that the `accountsChanged`
event should emit an array of strings.
Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

Yup, I just published that version of inpage-provider.

@metamaskbot
Copy link
Collaborator

Builds ready [3c6e5fb]

@Gudahtt Gudahtt merged commit 95b4d91 into develop Sep 13, 2019
Gudahtt added a commit that referenced this pull request Sep 13, 2019
* Replace `undefined` selectedAddress with `null`

The `runtime.Port.postMessage` API will drop keys with a value of
`undefined` on Chrome, but not on Firefox. This was a problem for the
`publicConfig` stream, which passed the key `selectedAddress` with the
value of `undefined` to communicate to dapps that the user had logged
out.

Instead a `null` is now passed for `selectedAddress` upon logout, which
is correctly sent by the `runtime.Port.postMessage` API on both Chrome
and Firefox.

closes #7101
closes #7109

* Update `metamask-inpage-provider` to v3.0.0

The v3.0.0 update includes a change to the `accountsChanged` event. The
event will now emit an empty array instead of an array with `undefined`
or `null`.

The previous behavior was to emit `[undefined]`. The previous commit
would have changed that to `[null]` anyway, so we figured if we're
going to make a public-facing change to the event anyway we should
change it to be correct. `[undefined]` was never intended, and it
technically violates EIP-1193, which states that the `accountsChanged`
event should emit an array of strings.
Gudahtt added a commit that referenced this pull request Sep 17, 2019
…evelop

* origin/develop: (31 commits)
  Performance: Delivery optimized images (#7176)
  Add `appName` message to each locale
  Remove the disk store (#7170)
  Update @hapi/subtext as per security advisory (#7172)
  Add fixes for German translations (#7168)
  Fix recipient field of approve screen (#7171)
  3box integration 2.0 (#6972)
  ci - metamaskbot - include links to dep-viz and all artifacts (#7155)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Remove redundant error logging (#7158)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  ci - install deps with "--har" flag to capture network activity (#7143)
  ci - create source-map-explorer build-artifacts (#7141)
  ci - build-artifacts - generate sesify-viz for inspecting deps (#7151)
  Publish GitHub release from master branch (#7136)
  fix rinkeby spelling (#7148)
  deps - move gulp-terser-js to devDeps
  test:integration - fix renamed test data file
  lint fix
  ...
Gudahtt added a commit that referenced this pull request Sep 27, 2019
* origin/master:
  Add v7.2.2 to changelog
  Update minimum Firefox verison to 56.0 (#7213)
  Version v7.2.2
  Update changelog for v7.2.1, v7.2.0, and v7.1.1
  Add `appName` message to each locale
  Version v7.2.1
  Update changelog with additional bug fixes
  Fix recipient field of approve screen (#7171)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  Publish GitHub release from master branch (#7136)
  Update the changelog for v7.1.1 (#7145)
  build - replace gulp-uglify-es with gulp-terser-js
  Version v7.2.0
  Update changelog with 7.2.0 changes
  Allow dismissing privacy mode from popup
  Add changelog
  Version v7.1.1
@Gudahtt Gudahtt deleted the replace-undefined-selectedAddress-with-null branch October 2, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ethereum.selectedAddress is not reset after Sign Out "accountsChanged" event not fired upon logout (Chrome)
4 participants