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

Fix wrong messages during MWP Flow #6094

Merged
merged 9 commits into from
Sep 10, 2024
Merged

Fix wrong messages during MWP Flow #6094

merged 9 commits into from
Sep 10, 2024

Conversation

brunobar79
Copy link
Member

@brunobar79 brunobar79 commented Sep 9, 2024 •
edited
Loading

Fixes APP-1857
Fixes APP-1854

What changed (plus any additional context for devs)

A few things were happening with MWP causing this issue:

  • We were incorrectly handling the keychain errors so any cancellation would trigger the alert about phones swapping.

  • Sometimes (?) we were looking up the pkey using a lowercase version while the original is stored checksummed resulting in "unavailable" error. I am not sure why but I believe this was on Warpcast side sending different versions of the address, which is why reconnecting fixed the issue (they sent the right one there) but somewhere else they were breaking it. We're now checksumming it so we don't care anymore.

  • We were passing through a chainId parameter that was making the L1 fee estimation fail

Screen recordings / screenshots

RPReplay_Final1725922600.MP4

What to test

  • MWP normal tx signing flow
  • MPW cancelling authentication during face id and later pin.
  • Normal sends with and without faceID cancellation

jinchung
jinchung approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

🌮

@linear Linear
Copy link

linear bot commented Sep 9, 2024

APP-1857 Confirm button not working for MWP tx on Android

APP-1854 wrong error message for MWP transaction issue

@brunobar79 brunobar79 marked this pull request as ready for review September 9, 2024 22:59
@brunobar79 brunobar79 changed the title add logging for pkey errors Fix wrong messages during MWP Flow Sep 9, 2024
walmat
walmat approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

legend. tysm @brunobar79

@brunobar79
Copy link
Member Author

Launch in simulator or device for f46b6e9

brunobar79
brunobar79 commented Sep 10, 2024
View reviewed changes
src/keychain/index.ts Outdated Show resolved Hide resolved
jinchung
jinchung reviewed Sep 10, 2024
View reviewed changes
src/keychain/index.ts Outdated Show resolved Hide resolved
@brunobar79
Copy link
Member Author

Launch in simulator or device for 5b59cdf

@jinchung jinchung merged commit 82dca43 into develop Sep 10, 2024
8 checks passed
@jinchung jinchung deleted the @bruno/add-more-logging-pkey branch September 10, 2024 17:57
BrodyHughes pushed a commit that referenced this pull request Sep 10, 2024
@brunobar79 @BrodyHughes
Fix wrong messages during MWP Flow (#6094)
8e24521 
* add logging for pkey errors

* fix keychain error handling

* checksum address before keychain lookup

* remove invalid chainId param from tx

* Update src/keychain/index.ts

* Update src/keychain/index.ts

Co-authored-by: Jin <[email protected]>

* Explicitly return UserCanceled error for code strings 10 and 13

* Remove returning -3 explicit error code

* Use enum values instead of primitives for error codes for readability

---------

Co-authored-by: Jin <[email protected]>
(cherry picked from commit 82dca43)
@sentry-io Sentry.io
Copy link

sentry-io bot commented Sep 10, 2024 •
edited
Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: KC unknown error for PKEY lookup _callSuper(src/logger/index) View Issue
  • ‼️ Error: KC unknown error for PKEY lookup <global>(src/logger/index) View Issue
  • ‼️ Error: you passed empty or null username/password setInternetCredentials(index.android) View Issue
  • ‼️ Error: KC unknown error for PKEY lookup <global>(src/logger/index) View Issue

Did you find this useful? React with a 👍 or 👎

greg-schrammel pushed a commit that referenced this pull request Sep 11, 2024
@brunobar79 @jinchung @greg-schrammel
Fix wrong messages during MWP Flow (#6094)
b2f6434 
* add logging for pkey errors

* fix keychain error handling

* checksum address before keychain lookup

* remove invalid chainId param from tx

* Update src/keychain/index.ts

* Update src/keychain/index.ts

Co-authored-by: Jin <[email protected]>

* Explicitly return UserCanceled error for code strings 10 and 13

* Remove returning -3 explicit error code

* Use enum values instead of primitives for error codes for readability

---------

Co-authored-by: Jin <[email protected]>
walmat added a commit that referenced this pull request Sep 13, 2024
@greg-schrammel @ibrahimtaveras00
@BrodyHughes @brunobar79 @jinchung @walmat @dependabot @estebanmino @benisgold
fix degen <-> wdegen (#6087)
64d1387 
* add degen native asset

* remove NATIVE_ASSETS_MAP_PER_CHAIN

* remove Address castings

* bump iOS and Android version to v1.9.38 (#6081)

* Fix e2e flakiness / errors (#6084)

* fix

* test

* revert

* @walmat related fixes

* @walmat related fixes

* okay disabling the test again, the experimental flag didnt fix it

* okay disabling the test again, the experimental flag didnt fix it

* fix disabled NFTs in testing and rely on remote config for prod

* .

* add 60 min timeout (#6090)

* fix tophat triggers (#6089)

* Fix url creation for stale balance param (#6091)

* fix wrong id (#6078)

* change to id

* also fix clicks

* Add rc-push script (#6088)

* add rc-push script

* source .env

* Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows (#6072)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 3 to 4.1.7.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v3...v4.1.7)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fixes opacity on mwp sign txn sheet (#6083)

* change opacity on mwp route source too

* switch to includes

* update env (#6077)

* fix missing dapp metadata for eth actions (#6086)

* fix missing dapp metadata for eth actions

* use isHandshakeAction helper function

* Update src/components/MobileWalletProtocolListener.tsx

Co-authored-by: Bruno Barbieri <[email protected]>

---------

Co-authored-by: Bruno Barbieri <[email protected]>

* Revert "Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows …" (#6095)

This reverts commit 431459a.

* Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows (#6072)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 3 to 4.1.7.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v3...v4.1.7)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Revert "Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows …" (#6095)

This reverts commit 431459a.

* Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows (#6072)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 3 to 4.1.7.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v3...v4.1.7)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Revert "Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows …" (#6095)

This reverts commit 431459a.

* fix: send (#6093)

* fix

* lint

* userAssetsStore refactor (#6015)

* done

* change userAssetsStoreCache type

* mid refactor

* refactor

* dont cast to Address

* more

* more

* rm address param

* fix caching

* select address

* comments

* hardhat

* lint

* Fix wrong messages during MWP Flow (#6094)

* add logging for pkey errors

* fix keychain error handling

* checksum address before keychain lookup

* remove invalid chainId param from tx

* Update src/keychain/index.ts

* Update src/keychain/index.ts

Co-authored-by: Jin <[email protected]>

* Explicitly return UserCanceled error for code strings 10 and 13

* Remove returning -3 explicit error code

* Use enum values instead of primitives for error codes for readability

---------

Co-authored-by: Jin <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Ibrahim Taveras <[email protected]>
Co-authored-by: brdy <[email protected]>
Co-authored-by: Bruno Barbieri <[email protected]>
Co-authored-by: Jin <[email protected]>
Co-authored-by: Matthew Wall <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Esteban Miño <[email protected]>
Co-authored-by: Ben Goldberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinchung jinchung jinchung approved these changes

@walmat walmat walmat approved these changes

@ibrahimtaveras00 ibrahimtaveras00 Awaiting requested review from ibrahimtaveras00

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brunobar79 @jinchung @walmat