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.

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

chore: Resolve conflict v12.4.0 #27541

Merged
merged 8 commits into from
Oct 1, 2024
Merged

Conversation

chloeYue
Copy link
Contributor

@chloeYue chloeYue commented Oct 1, 2024

Description

Resolve conflict v12.4.0

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

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.

metamaskbot and others added 8 commits September 26, 2024 18:50
… (#27435)

This PR correctly applies the changes from #26807 and #26924 to master.
These were broken, probably by careless human error, in 3e1094a

---------

Co-authored-by: Matteo Scurati <[email protected]>
## **Description**

Fixes issue
[27456](#27456) in
12.3.

If you try to add a new network with the same RPC URL and chain id as an
existing network, the link presented in the error message does not work
and throws an error:


https://github.com/user-attachments/assets/ebcade54-e49e-4a04-9ed6-396f2458f77a


This link in the error message was intended for the network redesign and
should stay behind the feature flag. Duplicate chain IDs are allowed in
this version of network managment. So this PR makes it back into a
warning like it would have been in 12.2:

<img width="338" alt="Screenshot 2024-09-27 at 12 02 49 PM"
src="https://github.com/user-attachments/assets/8c114cf7-5a1d-4662-b84f-a74caaef08d8">

There's nothing to fix in develop since it is already on the new network
controller / UI

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

## **Related issues**

Fixes: #27456

## **Manual testing steps**

1. Try to add a new network with the same RPC URL and chain id as an
existing network.

## **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/develop/.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/develop/.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.
…ring error (#27457) (#27462)

## **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?
-->

The original code assumes that getMethodData will always return an
object with a name property. However, in certain instances,
getMethodData can return null. When this happens, destructuring the name
property from null causes a runtime error.

To address this issue, the code has been updated to use optional
chaining. This ensures that if getMethodData returns null, the
destructuring will not occur, and contractMethodName will be set to
undefined instead of causing an error.

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

## **Related issues**

Fixes: #27436

## **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/develop/.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/develop/.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: Pedro Figueiredo <[email protected]>
## **Description**

Update changelog for v12.3.1

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

## **Related issues**

N/A

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **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/develop/.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/develop/.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.
## **Description**

The "Fix notification metrics" change entry linked a PR where some of
these metrics were originally introduced, but it should have linked the
PR where they were fixed. The link has been updated.

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

## **Related issues**

N/A

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **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/develop/.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/develop/.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.
@chloeYue chloeYue self-assigned this Oct 1, 2024
@chloeYue chloeYue requested review from a team as code owners October 1, 2024 15:32
Copy link
Contributor

github-actions bot commented Oct 1, 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.

@chloeYue chloeYue changed the title Resolve conflict v12.4.0 chore: Resolve conflict v12.4.0 Oct 1, 2024
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit bc08781 into Version-v12.4.0 Oct 1, 2024
42 of 58 checks passed
@Gudahtt Gudahtt deleted the resolve-conflict-12.4.0 branch October 1, 2024 15:38
@github-actions github-actions bot locked and limited conversation to collaborators Oct 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [99811e0]
Page Load Metrics (1633 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30118001560300144
domContentLoaded1489179116048038
load1498180416339144
domInteractive136529136

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants