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: Merge master into v12.5.0 (following v12.4.2) #27938

Merged
merged 8 commits into from
Oct 17, 2024
Merged

chore: Merge master into v12.5.0 (following v12.4.2) #27938

merged 8 commits into from
Oct 17, 2024

Conversation

hjetpoluru
Copy link
Contributor

@hjetpoluru hjetpoluru commented Oct 17, 2024 •
edited by github-actions bot
Loading

Description

PR to merge hotfix changes of v12.4.2 into v12.5.0

Open in GitHub Codespaces

metamaskbot and others added 5 commits October 15, 2024 20:15
@metamaskbot
Version v12.4.2
516af04
@FrederikBolding @hmalik88
fix: Correct conditions for triggering name lookup during send (#27880)
e4672a8 
<!--
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.
-->

Take changes from
5e08c06

## **Description**

Hotfixes a problem preventing certain name resolution Snaps from being
triggered due to faulty `IS_FLASK` conditions in `12.4.1`. These
conditions were removed in
#26242. This PR picks
these changes from the previously mentioned PR, without touching the ENS
integration.

The problem in question occurs when trying to trigger name resolution
for a given input. `lookupDomainName` is never called on stable unless
the input looks similar to an ENS name. This prevents resolution of
inputs that don't use TLDs for instance.

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

## **Manual testing steps**

1. Use a build of stable
2. Install https://snaps.metamask.io/snap/npm/social-names-snap/
3. See that you can resolve a Farcaster name using that Snap, e.g.
`fc:frederik`
4. Check that you can also type a valid ENS name
5. Check that you can also type a valid address

---------

Co-authored-by: Hassan Malik <hbmalik88@gmail.com>
@FrederikBolding
chore: Update RC hotfix changelog (#27900)
9221f8e 
<!--
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**

Updates the hotfix RC changelog to mention the PR that was picked into
the RC.
@danjm @FrederikBolding
chore: Cherry pick f523617 to v12.4.2 (#27899)
36fe5a8 
Cherry picks f523617
(#27856) to v12.4.2
RC branch

Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
@Gudahtt
Merge pull request #27879 from MetaMask/Version-v12.4.2
626289d 
Version v12.4.2 RC
@hjetpoluru hjetpoluru requested a review from danjm October 17, 2024 16:05
@hjetpoluru hjetpoluru self-assigned this Oct 17, 2024
@hjetpoluru hjetpoluru requested a review from a team as a code owner October 17, 2024 16:05
@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.

benjisclowder
benjisclowder previously approved these changes Oct 17, 2024
View reviewed changes
danjm
danjm reviewed Oct 17, 2024
View reviewed changes
app/_locales/en/messages.json Outdated
@@ -4249,9 +4249,6 @@
"receive": {
"message": "Receive"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is present on develop, and so it probably should not be deleted: https://github.com/MetaMask/metamask-extension/blob/develop/app/_locales/en/messages.json#L4323-L4325

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you that the develop shows as posted by you.

But in v12.4.2 has this changes - https://github.com/MetaMask/metamask-extension/blob/develop/app/_locales/en/messages.json#L4323-L4325

Hence I took the v12.4.2. Now my current question is how did this code get to hotfix only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks @danjm, I have address the conflict with the instruction you have provided.

danjm
danjm reviewed Oct 17, 2024
View reviewed changes
app/_locales/en_GB/messages.json Outdated
@@ -4180,7 +4180,7 @@
"receive": {
"message": "Receive"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

On develop, this is "recipientAddressPlaceholder": https://github.com/MetaMask/metamask-extension/blob/develop/app/_locales/en_GB/messages.json#L4143-L4145

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you that the develop shows as posted by you.

But in v12.4.2 has this changes - https://github.com/MetaMask/metamask-extension/pull/27879/files#diff-7509d7529ea1cbc57ef79713676f7d124d9d5a446053f94836876c4cd3f3ccd6

Hence I took the v12.4.2. Now my current question is how did this code get to hotfix only?

@benjisclowder benjisclowder dismissed their stale review October 17, 2024 16:45

Additional modifications needed as per Dan's comments.

danjm
danjm reviewed Oct 17, 2024
View reviewed changes
ui/ducks/send/send.js Outdated
@@ -109,6 +109,7 @@ import {
isBurnAddress,
isPossibleAddress,
isValidHexAddress,
isPossibleAddress,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change probably needs to be removed for lint to pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure done.

danjm
danjm reviewed Oct 17, 2024
View reviewed changes
ui/ducks/send/send.test.js Outdated
@@ -768,7 +768,7 @@ describe('Send Slice', () => {
it('should not error with an invalid address error when user input is not a valid hex string', () => {
const tokenAssetTypeState = {
...INITIAL_SEND_STATE_FOR_EXISTING_DRAFT,
recipientInput: '0xValidateError',
recipientInput: '0xAAAA6BF26964aF9D7eEd9e03E53415D37aA96045',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This change probably needs to be removed for tests to pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok done.

@metamaskbot
Copy link
Collaborator

Builds ready [0e45523]
Page Load Metrics (1728 ± 108 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15923261573509244
domContentLoaded144522961709223107
load145223031728224108
domInteractive15171484421

@danjm danjm changed the title chore: Merge hotfix v12.4.2 chore: Merge master into v12.5.0 (following v12.4.2) Oct 17, 2024
@danjm danjm merged commit 154d25a into Version-v12.5.0 Oct 17, 2024
71 of 74 checks passed
@danjm danjm deleted the merge-hotfix-v12.4.2 branch October 17, 2024 19:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [9f77f8f]
Page Load Metrics (1807 ± 131 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint152927181804274132
domContentLoaded152226871782273131
load153027231807272131
domInteractive25192564321

@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Oct 21, 2024
@metamaskbot
Copy link
Collaborator

No release label on PR. Adding release label release-12.5.0 on PR, as PR was added to branch 12.5.0 when release was cut.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danjm danjm danjm approved these changes

@benjisclowder benjisclowder benjisclowder left review comments

Assignees

@hjetpoluru hjetpoluru

Labels
extension-delivery release-12.5.0 Issue or pull request that will be included in release 12.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hjetpoluru @metamaskbot @danjm @benjisclowder @FrederikBolding @Gudahtt