Skip to content

diagnostics - add multiple vault detection to diagnostics reporting #4506

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
Merged
merged 5 commits into from
Jun 5, 2018
Merged

diagnostics - add multiple vault detection to diagnostics reporting #4506

merged 5 commits into from
Jun 5, 2018

Conversation

@kumavis
Copy link
Member

@kumavis kumavis commented Jun 5, 2018

No description provided.

@metamaskbot
Copy link
Collaborator

Builds ready [ece5cfc]: mascara, chrome, firefox, edge, opera

bitpshr
bitpshr previously approved these changes Jun 5, 2018
View reviewed changes
Copy link
Contributor

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor nits / comments.

app/scripts/controllers/preferences.js

this.getFirstTimeInfo = opts.getFirstTimeInfo || null
this.notifier = opts.notifier || notifier
this.diagnostics = opts.diagnostics
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we default this to some fallback instance of the DiagnosticsReporter? Maybe this isn't necessary if we know it's always passed where this PreferencesController is instantiated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I see this is defaulted in MetaMaskController.

app/scripts/lib/diagnostics-reporter.js Outdated

async reportOrphans(orphans) {
try {
await this.submit({
Copy link
Contributor

@bitpshr bitpshr Jun 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You should be able to return this.submit(...) directly and this function doesn't have to be marked as async since it won't use await anymore. In either case, this will return a Promise that will resolve when the submission is complete.

Copy link
Member Author

Choose a reason for hiding this comment

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

the await here makes sure the try/catch catches errors coming from this.submit, tho we are catching all errors inside this.submit
we could remove the error handling in this.submit so we know which method errored

app/scripts/metamask-controller.js
const accounts = await this.keyringController.getAccounts()

// verify keyrings
const nonSimpleKeyrings = this.keyringController.keyrings.filter(keyring => keyring.type !== 'Simple Key Pair')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we lowercase this string comparison? I lack context so maybe it's unnecessary, just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

here its ok, as it (should be) one of 2 hard coded values

app/scripts/metamask-controller.js Outdated
// verify keyrings
const nonSimpleKeyrings = this.keyringController.keyrings.filter(keyring => keyring.type !== 'Simple Key Pair')
if (nonSimpleKeyrings.length > 1) {
if (this.diagnostics) await this.reportMultipleKeyrings(nonSimpleKeyrings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, these compound if statements can be combined.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair

bitpshr
bitpshr previously approved these changes Jun 5, 2018
View reviewed changes
@metamaskbot
Copy link
Collaborator

Builds ready [36a0574]: mascara, chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [60e61e6]: mascara, chrome, firefox, edge, opera

@kumavis kumavis merged commit 5b3af34 into develop Jun 5, 2018
@kumavis kumavis deleted the diagnostics-multivault branch June 5, 2018 19:51
github-merge-queue bot pushed a commit that referenced this pull request Mar 31, 2025
@matthewwalsh0
fix: simulation of type-4 transactions (#31335)
1bb5fae 
## **Description**

Fix balance change simulation of type-4 transactions by bumping
`@metamask/transaction-controller` to `52.3.0`.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31335?quickstart=1)

## **Related issues**

Fixes:
[#4506](MetaMask/MetaMask-planning#4506)

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
pedronfigueiredo pushed a commit that referenced this pull request Mar 31, 2025
@matthewwalsh0 @pedronfigueiredo
fix: simulation of type-4 transactions (#31335)
35ff1b0 
Fix balance change simulation of type-4 transactions by bumping
`@metamask/transaction-controller` to `52.3.0`.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31335?quickstart=1)

Fixes:
[#4506](MetaMask/MetaMask-planning#4506)

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.
MajorLift pushed a commit that referenced this pull request Apr 1, 2025
@pedronfigueiredo @matthewwalsh0
chore: Bump transaction controller version (#31440)
064270f 
Cherry-pick: #31335

This introduces three high-priority core repo PRs in this release:
- MetaMask/core#5511
- MetaMask/core#5549
- MetaMask/core#5552

--- // ---

Fix balance change simulation of type-4 transactions by bumping
`@metamask/transaction-controller` to `52.3.0`.

[![Open in GitHub

Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31335?quickstart=1)

Fixes:
[#4506](MetaMask/MetaMask-planning#4506)

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding

Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31440?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] 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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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: Matthew Walsh <matthew.walsh@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bitpshr bitpshr bitpshr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kumavis @metamaskbot @bitpshr