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.

Sign up for GitHub

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

Jump to bottom

[Bug]: Crash when signing message #7920

Closed
Gudahtt opened this issue Nov 23, 2023 · 2 comments · Fixed by #7917
Closed

[Bug]: Crash when signing message #7920

Gudahtt opened this issue Nov 23, 2023 · 2 comments · Fixed by #7917
Labels
type-bug Something isn't working

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Nov 23, 2023 •
edited
Loading

Describe the bug

The application crashes when signing an eth_signTypedData_v4 message on the main branch. Various Bitrise test runs hint that the same is true of other types of messages as well.

Expected behavior

The application should not crash when signing a message

Screenshots/Recordings

Screenshot 2023-11-23 at 10 11 02 AM

Steps to reproduce

  • Navigate to https://metamask.github.io/test-dapp using a build from main (tested with 69c79df)
    • I have tested this with a development build, I am not sure if that matters for this reproduction though
  • Connect an account to the dapp
  • Trigger an eth_signTypedData_v4 signature, and the application with crash

Error messages or log output

Initially the error shown is Attempting to define a property on object that is not extensible. However this error is itself caused by the underlying error, and is related to ses.

I removed ses locally to find the underlying error here, which was _imageSource$uri.endsWith is not a function. It is from this line:

metamask-mobile/app/util/favicon/index.ts

Line 163 in 69c79df

(imageSource.uri?.endsWith('.svg') ||

Version

69c79df

Build type

None

Device

iPhone 13 Pro simulator

Operating system

iOS

Additional context

No response

Severity

No response
@Gudahtt Gudahtt added the type-bug Something isn't working label Nov 23, 2023
@Gudahtt Gudahtt mentioned this issue Nov 23, 2023
Merged
13 tasks
@seaona
Copy link
Contributor

seaona commented Nov 23, 2023 •
edited
Loading

Thanks for logging this @Gudahtt 🙏 I've triggered a run for Confirmations tests and indeed this is catched by our tests. We have a PR that flags all the Confirmations tests (including Signatures) as Smoke tests, so they are run on every PR.
Hopefully we can get it merge soon, so we can capture more of these issues ahead of time. 🙏

Screenshot from 2023-11-23 15-41-39
https://app.bitrise.io/build/fa65a941-6cd6-4b0c-9ece-151e76cdbb0d

@seaona seaona mentioned this issue Nov 23, 2023
Merged
13 tasks
This was referenced Nov 23, 2023
Merged
Closed
Merged
@Gudahtt Gudahtt linked a pull request Nov 24, 2023 that will close this issue
fix: adjust the apiLogoUrl logic to handles cases when icon is an object with uri key #7917
Merged
13 tasks
@Gudahtt
Copy link
Member Author

Gudahtt commented Nov 24, 2023

Fixed by #7917

@Gudahtt Gudahtt closed this as completed Nov 24, 2023
Gudahtt added a commit that referenced this issue Nov 24, 2023
@Gudahtt
fix: Prevent SES errors in iOS dev builds (#7924)
0163b8b 
## **Description**

SES is now disabled in development builds. This was done as a workaround
to various incompatibilities between SES and React development
libraries, which were causing a SES error whenever a warning or error
was triggered in an iOS dev build.

## **Related issues**

Fixes #7923

## **Manual testing steps**

Follow the reproduction steps for this issue:
#7920

## **Screenshots/Recordings**

Before:

<img width="427" alt="Screenshot 2023-11-23 at 7 00 47 PM"
src="https://github.com/MetaMask/metamask-mobile/assets/2459287/11143d5c-f2ce-4594-a525-e74d52e96255">


After:

<img width="427" alt="Screenshot 2023-11-23 at 6 54 32 PM"
src="https://github.com/MetaMask/metamask-mobile/assets/2459287/3fd72105-28fc-4cdc-af50-61e9da3da780">

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
seaona added a commit that referenced this issue Dec 4, 2023
@seaona @vinistevam
test: flag Confirmations tests as Smoke tests (#7710)
bc3ea74 
## **Description**
In this PR we are flagging all the Confirmations tests as Smoke test.
This includes the following spec files:

- advanced-gas (the only one that was previously flagged as Smoke)
- approve-default-erc
- eth-sign
- personal-sign
- send-erc20
- send-erc721
- send-eth
- typed-sign
- typed-sign-v3
- typed-sign-v4

Notice the amount of time added on the runs is not significant compared
to the amount of scenarios we add, giving everyone more confidence on
making changes in the Wallet. See numbers below.

Notice the tests are stable, being 100% Successful after several ci
runs. This was achieved thanks to [this
fix](#7802) by
@vinistevam

###  Recent examples how Confirmations tests captured an error on `main`
-
[Here](https://consensys.slack.com/archives/C02U025CVU4/p1699963068242849?thread_ts=1699950726.232009&cid=C02U025CVU4)
is an example of how running all the Confirmations tests, would have
captured and prevent an error in `main`. Luckily we were running them
locally and could spot the error and connect the dots with the slack
thread.

- [Here](#7920)
Signatures flow broken in main, that would have been captured by
Confirmations tests if run as Smoke
https://app.bitrise.io/build/fa65a941-6cd6-4b0c-9ece-151e76cdbb0d

### Test Runs Data
#### Run #1 - Detox Build & Test
- ios: 16m 31s
- android: 36m 09s
- bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/10633145-b793-4ac3-8d9b-316ca24586f8

#### Run #2 - Detox Build & Test
- ios: 18m 15s
- android: 34m 15s
- bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/aad945da-60b3-49f2-ae89-18a6a59efe35


#### Run #3 - Detox Build & Test
- ios: 17m 30s
- android: 36m 12s
- bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a9bd1bd3-a23f-4535-9b21-8475ca790980


#### Run #4 - Detox Build & Test
- ios: 18m 47s
- android: 35m 58s
- bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3109490d-0355-4eb3-bbed-bedd644f26d9

#### Run #5 - Detox Build & Test
- ios: 16m 57s
- android: 32m 02s
- bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/69a433eb-0bb0-4b67-95b2-c58145cc8963


## **Related issues**

Fixes: #

## **Manual testing steps**
Trigger ci test runs from Bitrise and see that all tests are passing.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@Gudahtt @seaona