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: Improve Keyring/Accounts error handling and logs #12822

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

chore: Improve Keyring/Accounts error handling and logs #12822

wants to merge 17 commits into from
+1,182 −43

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Dec 20, 2024 •
edited
Loading

Description

This PR does the following

  1. unmasks non sensitive data from the Accounts Controller and Keyring controller to help us better debug issues in prod. This means that more data will be provided in the sentry state logs. I have ensured that all addresses have remained masked
  2. adds logs when creating key components such as the engine and the accounts controller. This will help us debug issues with empty state.
  3. adds validation to the migrations for key components. Currently we are validating that the accounts controller and keyring controller data are sound. This validation will not block the app from initializing but will log the errors it finds.

Related issues

Progresses : #12408

Manual testing steps

  1. this change only adds logs and error handling to the app so there should be no functional changes. that being said, we should still verify that the upgrade path is working.

Screenshots/Recordings

Before

After

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.

@github-actions GitHub Actions
Copy link
Contributor

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.

@owencraston owencraston added the Run Smoke E2E Triggers smoke e2e on Bitrise label Dec 20, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 4c958ed
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/af7f2b81-5142-4ee2-a670-5cd7bfd5055d

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

@owencraston owencraston force-pushed the chore/improve-accounts-controller-error-logging branch from abf729d to ce694cc Compare December 21, 2024 18:17
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.53731% with 5 lines in your changes missing coverage. Please review.

Project coverage is 59.11%. Comparing base (9d6908b) to head (7c6f385).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
app/store/index.ts 0.00% 2 Missing ⚠️
app/core/Engine/Engine.ts 66.66% 1 Missing ⚠️
...re/Engine/controllers/AccountsController/logger.ts 75.00% 0 Missing and 1 partial ⚠️
app/core/Engine/utils/logger.ts 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #12822      +/-   ##
==========================================
+ Coverage   58.85%   59.11%   +0.26%     
==========================================
  Files        1851     1859       +8     
  Lines       41739    41876     +137     
  Branches     5397     5436      +39     
==========================================
+ Hits        24566    24757     +191     
+ Misses      15502    15444      -58     
- Partials     1671     1675       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

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

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 327980c
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8c2d0c55-c540-4a22-98f5-6627c740ba9c

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

@owencraston owencraston force-pushed the chore/improve-accounts-controller-error-logging branch from 327980c to 8a10efc Compare January 6, 2025 18:17
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 5d25241
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ffe3c313-980c-4c77-b869-34064dd47391

Note

  • This comment will auto-update when build completes
  • 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

@owencraston owencraston marked this pull request as ready for review January 6, 2025 21:00
@owencraston owencraston requested review from a team as code owners January 6, 2025 21:00
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: a7ab7b4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/607cad82-eaba-4a4b-91ff-2d2ef277d364

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a09ede8
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/aac38ca3-7d88-4b75-8a7b-64b03148e1fc

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

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jan 8, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
3 Accepted issues

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

See analysis details on SonarQube Cloud

@owencraston
Copy link
Contributor Author

Prod test build link. I created a new branch for the test build that has the version numbers bumped but the code is the same as this branch.

@owencraston
Copy link
Contributor Author

Tested the upgrade path from version 7.35.0 (since this seems to be a problematic version) to 7.38.1 which contains the code from this PR and everything worked as expected. I had a mix EOA and imported accounts with funds and all the data was accurate post upgrade.

ScreenRecording_01-08-2025.13-03-43_1.MP4

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 8, 2025
Cal-L
Cal-L approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Thanks for addressing comments. LGTM from platform

sethkfman
sethkfman approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@owencraston owencraston force-pushed the chore/improve-accounts-controller-error-logging branch from a09ede8 to 3f957ac Compare January 9, 2025 17:22
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 9, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 3f957ac
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f6800891-9381-4609-97c0-387c9182b06e

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

@owencraston owencraston enabled auto-merge January 9, 2025 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikesposito mikesposito mikesposito left review comments

@gantunesr gantunesr gantunesr left review comments

@Cal-L Cal-L Cal-L approved these changes

@sethkfman sethkfman sethkfman approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@owencraston @codecov-commenter @sethkfman @Cal-L @gantunesr @mikesposito @metamaskbot