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

chore: Chore/12435 mvp handle engine does not exist #12538

Merged
merged 45 commits into from
Dec 9, 2024
Merged

chore: Chore/12435 mvp handle engine does not exist #12538

merged 45 commits into from
Dec 9, 2024
+1,060 −603

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Dec 4, 2024 •
edited
Loading

Description

The main purpose of these changes is to ensure that the navigation service is available when Engine is initialized. The reason that this is needed is because it allows us to redirect instances of failed controller instances to vault recovery where users may at least recover their keys as opposed to being bricked. This is an example of the bricked state - #12115.

This PR reorganizes the initialization of services, including Engine to sagas, where it enables us to wait for dependencies to load first. The two dependencies that the Engine relies on are:

  • Persisted data loaded
  • Navigation loaded

Related issues

Fixes: #12435

Manual testing steps

While the underlying logic changes, the app behavior should remain the same

  1. Install the app on this branch
  2. Create a wallet
  3. Kill app, reopen and login
  4. Should not experience any issues

Screenshots/Recordings

Before

Simulates controller failed initialization

no.recovery.mov

After

Simulates vault recovery when controller fails to initialize
https://github.com/user-attachments/assets/76a853cd-34bc-465c-af03-d5c07609b8ee

Pre-merge author checklist

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.

Cal-L added 18 commits November 26, 2024 17:15
@Cal-L
Trigger on nav ready action
188fe55
@Cal-L
Create ReduxService to manage Redux store
c849f8a
@Cal-L
Move service initialization into sagas
fc90452
@Cal-L
Update EngineService to use ReduxService
369c615
@Cal-L
Update Authentication service to use ReduxService
149c8e4
@Cal-L
Update AppStateListener service to use ReduxService
469530b
@Cal-L
Update LockManager service to use ReduxService
f8c307f
@Cal-L
Move AssetPollingProvider to post login
3e91633
@Cal-L
Start services when events are ready
f597189
@Cal-L
Use types and enums in navigation reducer and actions
e4ca0c8
@Cal-L
Use types and enums for user reducer and actions
c07e908
@Cal-L
Set store earlier
6cca04c
@Cal-L
Add TODO for gate keeping based on service availability
8055bdb
@Cal-L
Clean up user and navigation reducers
58cdb78
@Cal-L
Give function space
7f002ae
@Cal-L
Abstract user actions
944ea95
@Cal-L
Abstract navigation actions
5bcbce5
@Cal-L
Update redux service import
62d6627
@Cal-L Cal-L requested review from a team as code owners December 4, 2024 05:08
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Cal-L Cal-L added No E2E Smoke Needed If the PR does not need E2E smoke test run Run Smoke E2E Triggers smoke e2e on Bitrise and removed No E2E Smoke Needed If the PR does not need E2E smoke test run labels Dec 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 92edb61
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b109a35c-2d64-4e9b-ad84-caf73cd8a6a4

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

tommasini
tommasini previously approved these changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM! I think a follow up work could be a e2e to check this recovery flow when something goes wrong, WDYT?

@Cal-L
Copy link
Contributor Author

Cal-L commented Dec 6, 2024

LGTM! I think a follow up work could be a e2e to check this recovery flow when something goes wrong, WDYT?

Agreed. We can probably achieve this by using a fixture with bad data

@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 6, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 6, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f184150
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/50ba249b-66bc-4cd9-b5fb-ebf8a9c55e54

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Cal-L
Cal-L commented Dec 6, 2024
View reviewed changes
app/core/EngineService/EngineService.ts Outdated Show resolved Hide resolved
app/store/index.ts Show resolved Hide resolved
@smilingkylan
Copy link
Contributor

LGTM 🚀

@smilingkylan smilingkylan self-requested a review December 6, 2024 22:25
smilingkylan
smilingkylan previously approved these changes Dec 6, 2024
View reviewed changes
@Cal-L Cal-L enabled auto-merge December 6, 2024 22:25
brianacnguyen
brianacnguyen previously approved these changes Dec 9, 2024
View reviewed changes
@Cal-L Cal-L dismissed stale reviews from brianacnguyen, smilingkylan, and tommasini via a81a617 December 9, 2024 21:08
@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a81a617
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9df02255-8405-401c-b3f3-c900994ff55a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

smilingkylan
smilingkylan approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@smilingkylan smilingkylan left a comment

Choose a reason for hiding this comment

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

Latest commit (fixing merge conflicts) looks fine 👍

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 9, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Cal-L Cal-L added this pull request to the merge queue Dec 9, 2024
Merged via the queue into main with commit 65e28fc Dec 9, 2024
39 of 40 checks passed
@Cal-L Cal-L deleted the chore/12435-mvp-handle-engine-does-not-exist branch December 9, 2024 22:02
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2024
@metamaskbot metamaskbot added the release-7.38.0 Issue or pull request that will be included in release 7.38.0 label Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vinnyhoward vinnyhoward vinnyhoward approved these changes

@owencraston owencraston owencraston approved these changes

@smilingkylan smilingkylan smilingkylan approved these changes

@tommasini tommasini tommasini left review comments

@brianacnguyen brianacnguyen brianacnguyen left review comments

Assignees
No one assigned
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-7.38.0 Issue or pull request that will be included in release 7.38.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle Engine does not exist failures as a result of failed controller initialization
7 participants
@Cal-L @smilingkylan @brianacnguyen @owencraston @vinnyhoward @tommasini @metamaskbot