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

fix: update usage of OP goerli to OP Sepolia #3999

Merged
merged 2 commits into from
Mar 4, 2024
Merged

fix: update usage of OP goerli to OP Sepolia #3999

merged 2 commits into from
Mar 4, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Feb 29, 2024 •
edited
Loading

Explanation

This PR replaces the reference to OP goerli to OP sepolia. It also renames the variable OPTIMISM_TESTNET to OPTIMISM_SEPOLIA.

Renaming the variable and switching to Sepolia is a breaking change, please use Sepolia and OPTIMISM_SEPOLIA instead of OPTIMISM_TESTNET.

References

Changelog

@metamask/transaction-controller

Removed

  • BREAKING: Renamed OPTIMISM_TESTNET to OPTIMISM_SEPOLIA and updated the chainId accordingly.
  • BREAKING: Updated the etherscan subdomain that was for OPTIMISM_TESTNET from goerli to sepolia.

@metamask/preferences-controller

Removed

  • BREAKING: Renamed OPTIMISM_TESTNET to OPTIMISM_SEPOLIA and updated the chainId accordingly.

@metamask/name-controller

Removed

  • BREAKING: Renamed OPTIMISM_TESTNET to OPTIMISM_SEPOLIA and updated the chainId accordingly.
  • BREAKING: updated the etherscan subdomain for OPTIMISM_TESTNET from goerli to sepolia

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@sahar-fehri sahar-fehri requested a review from a team as a code owner February 29, 2024 14:03
@sahar-fehri sahar-fehri force-pushed the feat/replace-optimism-goerli-with-sepolia branch from a1a89cc to 46c771a Compare February 29, 2024 14:05
@sahar-fehri sahar-fehri marked this pull request as draft February 29, 2024 14:06
@sahar-fehri sahar-fehri marked this pull request as ready for review February 29, 2024 15:21
@sahar-fehri sahar-fehri requested a review from Gudahtt February 29, 2024 17:26
Gudahtt
Gudahtt approved these changes Feb 29, 2024
View reviewed changes
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!

The contents look good, but I had a few suggestions for the PR description. Firstly it needs to be updated to reflect the rename. Second, I think this would quality as either an addition and a removal, or as a change, not as a fix. We're adding one variable and removing another, the old one is still "valid" for what it represents and was not a bug.

Also, could you indicate that the removal of OPTIMISM_TESTNET is a breaking change, and give advice on how to handle this change? (e.g. please switch to Sepolia instead and use OPTIMISM_SEPOLIA). Ideally all breaking changes would be accompanied by such advice.

Or alternatively, we could keep the old constant around and mark it as deprecated, rather than removing it, if you wanted to avoid the breaking change. Removing it seems fine to me though. We have breaking changes lined up for these packages already, so this can be grouped with those changes.

@sahar-fehri
Copy link
Contributor Author

LGTM!

The contents look good, but I had a few suggestions for the PR description. Firstly it needs to be updated to reflect the rename. Second, I think this would quality as either an addition and a removal, or as a change, not as a fix. We're adding one variable and removing another, the old one is still "valid" for what it represents and was not a bug.

Also, could you indicate that the removal of OPTIMISM_TESTNET is a breaking change, and give advice on how to handle this change? (e.g. please switch to Sepolia instead and use OPTIMISM_SEPOLIA). Ideally all breaking changes would be accompanied by such advice.

Or alternatively, we could keep the old constant around and mark it as deprecated, rather than removing it, if you wanted to avoid the breaking change. Removing it seems fine to me though. We have breaking changes lined up for these packages already, so this can be grouped with those changes.

Thanks @Gudahtt for pointing that out! I ve updated the description 🙏
I think we can keep the renaming the variable. I have checked extension, although we have the variable OPTIMISM_TESTNET but it is not imported from core, i can have another PR on extension to rename it to OPTIMISM_SEPOLIA so its more explicit too.

@sahar-fehri sahar-fehri force-pushed the feat/replace-optimism-goerli-with-sepolia branch from c2de8ae to 1fa721b Compare February 29, 2024 18:29
@MajorLift
Copy link
Contributor

MajorLift commented Mar 1, 2024 •
edited
Loading

What do you think about highlighting the breaking changes in the changelog as well?

Also, would it make sense to split the existing entries into "Removed" and "Added" entries, or do you think having a single "rename" entry for each makes more sense?
Edit: Restored original categories, since sepolia is the successor to goerli.

Changelog

@metamask/transaction-controller

Removed

  • BREAKING: Remove OPTIMISM_TESTNET from CHAIN_IDS and replace with OPTIMISM_SEPOLIA and corresponding chain ID.
  • BREAKING: Remove OPTIMISM_TESTNET and goerli subdomain from ETHERSCAN_SUPPORTED_NETWORKS and replace with OPTIMISM_SEPOLIA and sepolia subdomain.

@metamask/preferences-controller

Removed

  • BREAKING: Remove OPTIMISM_TESTNET from CHAIN_IDS and replace with OPTIMISM_SEPOLIA and corresponding chain ID.

@metamask/name-controller

Removed

  • BREAKING: Remove OPTIMISM_TESTNET from CHAIN_IDS and replace with OPTIMISM_SEPOLIA and corresponding chain ID.
  • BREAKING: Remove OPTIMISM_TESTNET and goerli subdomain from ETHERSCAN_SUPPORTED_NETWORKS and replace with OPTIMISM_SEPOLIA and sepolia subdomain.

@MajorLift MajorLift added enhancement New feature or request and removed enhancement New feature or request labels Mar 1, 2024
@sahar-fehri sahar-fehri force-pushed the feat/replace-optimism-goerli-with-sepolia branch from 1fa721b to f143250 Compare March 4, 2024 12:38
@sahar-fehri sahar-fehri merged commit a680605 into main Mar 4, 2024
139 checks passed
@sahar-fehri sahar-fehri deleted the feat/replace-optimism-goerli-with-sepolia branch March 4, 2024 12:43
mcmire pushed a commit that referenced this pull request Mar 4, 2024
@sahar-fehri @mcmire
fix: update usage of OP goerli to OP Sepolia (#3999)
4706c98 
## Explanation

This PR replaces the reference to OP goerli to OP sepolia. It also
renames the variable `OPTIMISM_TESTNET` to `OPTIMISM_SEPOLIA`.

Renaming the variable and switching to Sepolia is a breaking change,
please use Sepolia and `OPTIMISM_SEPOLIA` instead of `OPTIMISM_TESTNET`.

## References

* Fixes MetaMask/mobile-planning#1569
* Related to
[#67890](MetaMask/metamask-mobile#8784)

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/transaction-controller`

**Removed**

- **BREAKING**: Renamed `OPTIMISM_TESTNET` to `OPTIMISM_SEPOLIA` and
updated the chainId accordingly.
- **BREAKING**: Updated the etherscan subdomain that was for
OPTIMISM_TESTNET from goerli to sepolia.

### `@metamask/preferences-controller`

**Removed**

- **BREAKING**: Renamed `OPTIMISM_TESTNET` to `OPTIMISM_SEPOLIA` and
updated the chainId accordingly.

### `@metamask/name-controller`

**Removed**

- **BREAKING**: Renamed `OPTIMISM_TESTNET` to `OPTIMISM_SEPOLIA` and
updated the chainId accordingly.
- **BREAKING**: updated the etherscan subdomain for OPTIMISM_TESTNET
from goerli to sepolia

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
@sahar-fehri sahar-fehri mentioned this pull request Mar 4, 2024
Merged
13 tasks
Gudahtt added a commit that referenced this pull request Mar 5, 2024
@Gudahtt
Merge remote-tracking branch 'origin/main' into batch-queued-requests
ac8b75f 
* origin/main:
  refactor(controller-utils): replace `any` with types for type safety (#3975)
  Release 123.0.0 (#4007)
  [token-detection-controller] Refactor `detectTokens` method (#3938)
  fix: update usage of OP goerli to OP Sepolia (#3999)
sahar-fehri added a commit to MetaMask/metamask-mobile that referenced this pull request Mar 6, 2024
@sahar-fehri
fix: update usage of OP goerli to OP Sepolia (#8784)
1097298 
## **Description**

This PR's replaces the reference to OP goerli to OP sepolia.
Users that switch to OP goerli should still see the warning, and if they
have already made transactions on OP goerli, they should still see them
on activity tab.

Including the /compare diff URLs on core for visibility:

Core PR: MetaMask/core#3999


MetaMask/core@patch/mobile-transaction-controller-6-1-0...fix/update-OP-testnet-to-OP-sepolia


MetaMask/core@patch/mobile-transaction-controller-7-1-0...fix/update-OP-testnet

## **Related issues**

Fixes: MetaMask/mobile-planning#1569

CORE PR: MetaMask/core#3999

## **Manual testing steps**

1. Add OP goerli to your networks
2. You should still see the warning of OP goerli
3. Create transactions on OP goerli
4. You should see them in your activity tab

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**


https://github.com/MetaMask/metamask-mobile/assets/10994169/abd9976a-04c0-4d37-8659-8a620b637240

### **After**


https://github.com/MetaMask/metamask-mobile/assets/10994169/3eb5e152-2a18-4ae2-914f-f9899a641e64


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees
No one assigned
Labels
team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sahar-fehri @MajorLift @Gudahtt