fix formatting of 32-byte strings in personal_sign #5878
Conversation
NoahZinsmeister
requested review from
alextsg,
danjm and
whymarrh
as code owners
|
I thought the integration test failures might have just been a local machine problem, but it appears not. If a maintainer with a better grasp of how those tests work could give me a tip as to how to fix them, I'd be grateful. |
|
@NoahZinsmeister thanks for the PR! We can certainly help you get the integration tests passing.
Is this what you're doing with this PR? (Apologies if this is obvious.) |
|
Thanks @whymarrh! So, in the current version of Metamask, the first element of the
In the UI, this parsed argument is decoded from bytes to UTF-8, and shown to the user. This has the unfortunate effect of display garbled nonsense when the argument to This PR changes that behavior as follows: in cases when the passed argument is a 32-length byte string, it is now treated as 'plaintext', and UTF-8 encoded so that it displays correctly in the UI! This should cover most cases when devs actually want their users to sign a 'plaintext' bytes string (since usually this string will be a 32-length hash of something). |
|
Cool, I think that makes sense. Where this is strictly a visual change, I think we should try to limit this to the UI—can we add this logic to |
|
Sure thing, pushed. The only downside is that now hex-encoded UTF8 strings which happen to be 32 bytes will show as hex. A small price to pay, IMO. |
* Adds new gas customization modal container (without content)
* Adds the content of the advanced tab - w/o chart or dynamic content - to gas customize modal.
* Use correct message key in gas-modal-page-container.component.js
* Use BEM for css in gas-modal-page-container
* Split advanced-tab-content.component.js render() method into smaller pieces; add translations to the same file.
* Remove gas slider from advance-tab-content.component
* Add tests for advanced-tab-component.js and subcomponents.
* Improve styling of advanced-tab-content gasInput row
* Adds basic tab content to gas customizer, with styled button group (static, for now).
* Connect the gas-button-group component to redux and a live api.
* Improvements to propdefaults in button-group.component and basic-tab-content.component
* Styling fixes for gas customization advanced tab content.
* Adds gas-duck.test.js tests.
* Connects remained of the gas customization component to redux.
* Integrate gas buttons with the send screen.
* Test updates and additions for button integration with send screen.
* Adds redesign for the customize gas advanced tab.
* Adds not yet functional gas price chart.
* Gas price chart improvements, redesign, bug fixes, and set up to receive external data
* Read only connection of gas price chart to redux
* Clean up for advanced gas tab customization changes.
* Complete integration of gas chart with redux.
* Add control arrows to advanced gas tab inputs.
* Lint and unit test fixes.
* Clean up gas chart code.
* Update tests, plus some lint fixes, for gas-price-chart
* Improve data management and tests for gas-modal-page-container price estimates.
* Clean up for mmui-i11-custom-gas-price-chart branch
* Redesign of gas customization basic tab.
* Adds createSpeedUpTransaction to txController
* Connect gas price chart to gas station api.
* Adds speed up slide-in gas customization sidebar
* Update e2e tests for new gas customization modal.
* Fixes for components that break e2e gas customization tests, plus unit test updates.
* Remove gas customization integration tests (in favour of e2e tests)
* Add gas data to integration test json data set.
* Add c3 and d3 to the separate dependencies bundle.
* Make gas customization modal responsive.
* Fix "fastest" translation message; change to sentence case
* Uses more reliable api on main send screen; caches basic api results in modal
* Add loading spinners when waiting for APIs in the gas customization modal
* Modify results of API data to better fit gas chart: remove outliers, pad data
* Clear custom gas data on hiding of gas customization modal.
* Improve responsiveness of customize speed up slider.
* Final gas customization fixes
* Fix styling of send screen in extension view when hex data on.
* Replace height: 100% rule with workaround for flexbox quirks
* Fill in more Polish message translations
* Update lockfile to fix errors
npm has informed me that the lockfile has "errors":
npm ERR! code ELOCKVERIFY
npm ERR! Errors were found in your package-lock.json, run npm install to fix them.
npm ERR! Missing: c3@^0.6.7
npm ERR! Invalid: lock file's d3@3.5.17 does not satisfy d3@^5.7.0
* circleci: Disable npm audit when installing packages
Auditing packages when installing here doesn't help anyone as the summary
isn't visible and vulnerabilities don't produce a non-zero exit code. We
will have `npm audit` as an extra CI job.
* npm audit fix
* circleci: Replace nsp with npm audit
Refs #4751
* Remove beefy dependency and its usages
Refs #4768
Refs #5389
This changeset removes the beefy package that:
1. Was last published 2 yrs ago
2. Brought with it 1 moderate and 1 critical vulnerability
3. Was only used in scripts that no longer work
* npm uninstall open
* Update ganache-core to mitigate vuln
=== npm audit security report ===
> # Run npm install --save-dev ganache-core@2.3.1 to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate │ Memory Exposure │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ bl │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ ganache-core [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ ganache-core > level-sublevel > levelup > bl │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/596 │
└───────────────┴──────────────────────────────────────────────────────────────┘
* Deduplicate package.json file
From `npm install`:
> npm WARN The package css-loader is included as both a dev and production dependency.
> npm WARN The package eslint-plugin-react is included as both a dev and production dependency.
> npm WARN The package file-loader is included as both a dev and production dependency.
> npm WARN The package gulp is included as both a dev and production dependency.
It's also worth noting that the Gulp version we were using was inconsistent and there is
a published v4 release on GitHub.
* Fix race condition in network controller lookup() method.
* Group transactions by nonce (#5886)
* fix formatting of 32-byte strings in personal_sign (#5878)
* Bump json-rpc-engine to v4.0.0
* Bump package lock, mostly to https links
* Improve ux for low gas price set (#5862)
* Show user warning if they set gas price below safelow minimum, error if 0.
* Properly cache basic price estimate data.
* Default retry price to recommended price if original price was 0x0
* Use mock fetch in send-new-ui integration tests.
* Show Failed transaction in the browser notification for on-chain failures (#5904)
* Changelog and version bump for 5.2.0
Addresses #5473 and #3931. Formats 32-length byte strings passed to
personal_signas utf8 so that they are correctly displayed to users in the UI.In my opinion, it would be ideal for hex-encoded arguments passed to
personal_signto simply be displayed in the native bytes representation, and for non-hex-encoded arguments to be cast to utf8 bytes, but that's just my opinion.