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

fix(linea): enable token detection for Linea mainnet and testnet #20698

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

gauthierpetetin
Copy link
Contributor

@gauthierpetetin gauthierpetetin commented Sep 2, 2023

Explanation

Token detection was never enabled in the extension code although the APIs we rely on for token detection do support Linea Mainnet and Linea Goerli.

Work needed:

  • Add Linea Mainnet and Linea Goerli to the list of supported networks for autodetection (several enums to update in Extension code base).
  • Deploy a smart contract on Linea Mainnet and Linea Goerli. This contract is then used to fetch token balances in batch. Its address needs to be hardcoded in the Extension code base. See assetsContractController.js in the "Core" code base.

Screenshots/Screencaps

Before

Screenshot 2023-09-01 at 08 28 59

After

TBD

Manual Testing Steps

  1. Install Extension and initialise it with an account that already owns some well-known ERC20 tokens (Ex: USDC) on both Linea Mainnet and Linea Goerli.
  2. Go to "Extension Settings > Security & privacy > Token autodetection" and enable "Autodetect tokens" toggle.
  3. Select "Linea Mainnet" => Acceptance criteria: You shall see your USDC listed on the tokens list screen.
  4. Got to "Import tokens > Custom token" screen => Acceptance criteria: You shall not see the following banner: "Token detection is not available on this network yet."
  5. Import token with address 0x176211869cA2b568f2A7D4EE941E073a821EE1ff and symbol USDC => Acceptance criteria: Successful import.
  6. Select "Linea Goerli" => Acceptance criteria: You shall see your USDC listed on the tokens list screen.
  7. Got to "Import tokens > Custom token" screen => Acceptance criteria: You shall not see the following banner: "Token detection is not available on this network yet."
  8. Import token with address 0xb4257f31750961c8e536f5cfcbb3079437700416 and symbol USDC => Acceptance criteria: Successful import.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

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.

Token detection was never enabled in the extension code although the APIs we rely on for token detection do support Linea mainnet and testnet.
@gauthierpetetin gauthierpetetin force-pushed the fix/linea-token-detection branch from 94517fa to 571f6f9 Compare September 2, 2023 12:29
@metamaskbot
Copy link
Collaborator

Builds ready [571f6f9]
Page Load Metrics (1515 ± 32 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint102188130189
domContentLoaded1417165215136330
load1417165215156632
domInteractive1417165215136330
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 183 Bytes (0.00%)
  • common: 61 Bytes (0.00%)

Copy link
Contributor

@montelaidev montelaidev left a comment

Choose a reason for hiding this comment

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

lgtm

@chloeYue
Copy link
Contributor

chloeYue commented Sep 6, 2023

Test QA ok ! => Not OK, see comments below.

@gauthierpetetin
Copy link
Contributor Author

Thank you all for the reviews. @chloeYue I didn't personally manage to test this PR because I don't have a test account with ERC20 tokens on Linea Mainnet and Linea Goerli. Is there a test account we can use internally for such tests?

@chloeYue
Copy link
Contributor

Thank you all for the reviews. @chloeYue I didn't personally manage to test this PR because I don't have a test account with ERC20 tokens on Linea Mainnet and Linea Goerli. Is there a test account we can use internally for such tests?

Hello, we don't have testing account that have tokens on Mainnet, i could only test token detection on Linea Goerli with personal account.

@gauthierpetetin
Copy link
Contributor Author

I now have USDC on Linea Mainnet and Linea Goerli on a test account.

Here's the manual import feature, which works: https://recordit.co/O7wi2n8wxL

Here's the automated detection feature, which doesn't work on my side:
https://recordit.co/YUda9N3rr2

=> @chloeYue is it the automated detection feature that you managed to test?

@nkeywal
Copy link

nkeywal commented Sep 22, 2023

Hi there, what's the status? IIUC it can be merged but it has not been merged yet?

@chloeYue
Copy link
Contributor

QA update: I tested again on LINEA testnet, the token detection didn't work.

It's weird i had difference behaviour than last time, i'm not sure any more maybe the last time i did something wrong while testing. I removed the QA-passed label for this PR, i think we need more investigation.

cc: @gauthierpetetin

@gauthierpetetin gauthierpetetin added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) sprint-backlog and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 25, 2023
@gauthierpetetin
Copy link
Contributor Author

Hi @nkeywal ,
We've reviewed that ticket with the team on Monday.
The status is that the work needed to support token detection is more important than what's implemented in this PR.
=> @danjm has knowledge on what needs to be done and will document it in the coming days.

@nkeywal
Copy link

nkeywal commented Sep 27, 2023

Thanks a lot guys.

@danjm
Copy link
Contributor

danjm commented Oct 9, 2023

To achieve the goal of this PR, the detectNewTokens method needs to work on the chosen network: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/detect-tokens.js#L102

That means that isTokenDetectionEnabledForNetwork(chainIdAgainstWhichToDetect) needs to be true, this._tokenList.state needs to be populated, and this.assetsContractController.getBalancesInSingleCall needs to successfully return a result.

For isTokenDetectionEnabledForNetwork(chainIdAgainstWhichToDetect) to return true, modify the function in shared/modules/network.utils.ts to handle the chosen chainId

For this._tokenList.state to be populated, automatic token detection needs to be toggled on, and https://token-api.metaswap.codefi.network/tokens/${chainId} needs to be active and return a list of tokens

For this.assetsContractController.getBalancesInSingleCall to successfully return a result, then SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID in AssetsContractController.js of the the core repo's assets-controllers package needs to have a key-value pair for the chosen chainId. The key is the chainId, and the value is the address of a contract deployed on that network, a contract which meets the spec of the single-call-balance-checker-abi. This is the canonical repo for those contract https://github.com/wbobeirne/eth-balance-checker. So, to make this all work on Linea, of those contracts will need to be deployed to Linea. I believe someone in ConsenSys has done such a deployment in the past... possibly someone on the curated API team.

@gauthierpetetin
Copy link
Contributor Author

gauthierpetetin commented Oct 10, 2023

Thanks @danjm , here's a quick recap of what remains to be done:

modify the isTokenDetectionEnabledForNetwork function in shared/modules/network.utils.ts to handle the chosen chainId

=> ✅ This is done in this PR

https://token-api.metaswap.codefi.network/tokens/${chainId} needs to be active and return a list of tokens

=> ✅ This is done (see here for LINEA_MAINNET and here for LINEA_GOERLI)

SINGLE_CALL_BALANCES_ADDRESS_BY_CHAINID in AssetsContractController.js of the the core repo's assets-controllers package needs to have a key-value pair for the chosen chainId

=> 🚧 PR is open on core repo. It needs to be merged. Once merged, a new version of @metamask/assets-controllers package needs to be published and included in metamask-extension code base.

gauthierpetetin added a commit to MetaMask/core that referenced this pull request Oct 10, 2023
…i networks (#1799)

## Explanation

The purpose of this PR is to enable token detection on Linea Mainnet and
Linea Goerli networks.
[BalanceChecker](https://github.com/wbobeirne/eth-balance-checker) smart
contract has been deployed on both networks and its address needs to be
indicated in AssetsContractController.ts.

## References

* Unblocks MetaMask/metamask-extension#20698

## Changelog

### `'@metamask/assets-controllers`

- **ADDED**: Support token detection on Linea Mainnet and Linea Goerli
networks

## 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
@gauthierpetetin gauthierpetetin added release-11.4.0 Issue or pull request that will be included in release 11.4.0 release-blocker This bug is blocking the next release labels Oct 11, 2023
@gauthierpetetin
Copy link
Contributor Author

Adding release-blocker label to this issue as it needs to be included in release-11.4.0 (Linea team has a hard deadline for this on November 6th).

@gauthierpetetin
Copy link
Contributor Author

The PR on core repo got merged.
A new version of @metamask/assets-controllers package needs to be published but this is blocked by some dependencies issues in the core repo (more detail in this Slack discussion).
@mcmire is investigating these.

MajorLift pushed a commit to MetaMask/core that referenced this pull request Oct 11, 2023
…i networks (#1799)

## Explanation

The purpose of this PR is to enable token detection on Linea Mainnet and
Linea Goerli networks.
[BalanceChecker](https://github.com/wbobeirne/eth-balance-checker) smart
contract has been deployed on both networks and its address needs to be
indicated in AssetsContractController.ts.

## References

* Unblocks MetaMask/metamask-extension#20698

## Changelog

### `'@metamask/assets-controllers`

- **ADDED**: Support token detection on Linea Mainnet and Linea Goerli
networks

## 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
MajorLift pushed a commit to MetaMask/core that referenced this pull request Oct 11, 2023
…i networks (#1799)

## Explanation

The purpose of this PR is to enable token detection on Linea Mainnet and
Linea Goerli networks.
[BalanceChecker](https://github.com/wbobeirne/eth-balance-checker) smart
contract has been deployed on both networks and its address needs to be
indicated in AssetsContractController.ts.

## References

* Unblocks MetaMask/metamask-extension#20698

## Changelog

### `'@metamask/assets-controllers`

- **ADDED**: Support token detection on Linea Mainnet and Linea Goerli
networks

## 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
MajorLift pushed a commit to MetaMask/core that referenced this pull request Oct 12, 2023
…i networks (#1799)

## Explanation

The purpose of this PR is to enable token detection on Linea Mainnet and
Linea Goerli networks.
[BalanceChecker](https://github.com/wbobeirne/eth-balance-checker) smart
contract has been deployed on both networks and its address needs to be
indicated in AssetsContractController.ts.

## References

* Unblocks MetaMask/metamask-extension#20698

## Changelog

### `'@metamask/assets-controllers`

- **ADDED**: Support token detection on Linea Mainnet and Linea Goerli
networks

## 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
@gauthierpetetin
Copy link
Contributor Author

Here's a recording of the feature on Linea Mainnet: https://recordit.co/hJqmQJWy0g

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d6180fe) 68.61% compared to head (161100f) 68.60%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20698      +/-   ##
===========================================
- Coverage    68.61%   68.60%   -0.01%     
===========================================
  Files         1030     1030              
  Lines        41023    41029       +6     
  Branches     10946    10950       +4     
===========================================
+ Hits         28145    28147       +2     
- Misses       12878    12882       +4     
Files Coverage Δ
shared/modules/network.utils.ts 90.91% <100.00%> (+0.59%) ⬆️
ui/selectors/selectors.js 85.82% <0.00%> (-0.52%) ⬇️

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

@metamaskbot
Copy link
Collaborator

Builds ready [b9d9477]
Page Load Metrics (1010 ± 406 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85151108199
domContentLoaded691581042713
load8025211010845406
domInteractive691581042713
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 183 Bytes (0.00%)
  • common: 61 Bytes (0.00%)

@gauthierpetetin
Copy link
Contributor Author

gauthierpetetin commented Oct 13, 2023

Here's an example of response we receive from Token-API:

   {
      "address":"0x176211869ca2b568f2a7d4ee941e073a821ee1ff",
      "symbol":"USDC",
      "decimals":6,
      "name":"USD Coin",
      "iconUrl":"https://s2.coinmarketcap.com/static/img/coins/64x64/3408.png",
      "type":"erc20",
      "aggregators":[
         "lineaTeam",
         "coinGecko",
         "lifi",
         "rubic",
         "xswap"
      ],
      "occurrences":5
   }

You can notice occurrences is set to 5, because USDC si listed on 5 different aggregators.
The token detection feature works in a way, where tokens get detected only if token is listed on at least 3 aggregators (cf. this piece of code).

Due to this condition, tokens will never be detected on Linea Goerli, despite the feature being implemented, because there is only one single aggregator that supports Linea Goerli: "lineaTeam".

cc @danjm @pedronfigueiredo maybe we're fine with this behaviour, or maybe an alternative could be to remove this "3 aggregators" condition in case the network is a testnet. What do you think?

@gauthierpetetin gauthierpetetin added type-bug regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead labels Oct 13, 2023
@gauthierpetetin
Copy link
Contributor Author

gauthierpetetin commented Oct 20, 2023

This PR got unblocked by the upgrade of assets controllers to version 16 finalised by @pedronfigueiredo today.
@chloeYue this means this PR is ready to be re-tested.

@gauthierpetetin gauthierpetetin added needs-qa Label will automate into QA workspace and removed sprint-backlog labels Oct 20, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [161100f]
Page Load Metrics (947 ± 409 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint82137107168
domContentLoaded69153972211
load832076947851409
domInteractive69152972211
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 183 Bytes (0.00%)
  • common: 61 Bytes (0.00%)

@chloeYue
Copy link
Contributor

This PR got unblocked by the upgrade of assets controllers to version 16 finalised by @pedronfigueiredo today. @chloeYue this means this PR is ready to be re-tested.

Tested with the latest build, behaviour is as expected. Auto-detect works for Linea Mainnet. QA-passed for this PR.

Screen.Recording.2023-10-20.at.17.10.02.mov

@chloeYue chloeYue added QA Passed and removed needs-qa Label will automate into QA workspace labels Oct 20, 2023
@gauthierpetetin gauthierpetetin merged commit b9f75cf into develop Oct 20, 2023
11 of 13 checks passed
@gauthierpetetin gauthierpetetin deleted the fix/linea-token-detection branch October 20, 2023 15:15
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2023
@metamaskbot metamaskbot added the release-11.5.0 Issue or pull request that will be included in release 11.5.0 label Oct 20, 2023
@danjm danjm removed the release-11.5.0 Issue or pull request that will be included in release 11.5.0 label Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead release-11.4.0 Issue or pull request that will be included in release 11.4.0 release-blocker This bug is blocking the next release team-extension-platform team-linea Linea team type-bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants