Skip to content

Fix swaps when initial network not Mainnet#9745

Merged
Gudahtt merged 2 commits intodevelopfrom
fix-fetching-of-swaps-when-initial-network-not-mainnet
Oct 28, 2020
Merged

Fix swaps when initial network not Mainnet#9745
Gudahtt merged 2 commits intodevelopfrom
fix-fetching-of-swaps-when-initial-network-not-mainnet

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Oct 28, 2020

This is a continuation of #9726, which did not fix the problem described.

If the initial network when the extension is started is something other than Mainnet, the swaps controller will never successfully retrieve swap quotes. This is because ethers will continue to communicate with whichever network the provider was initially on.

We tried fixing this by hard-coding the chainId to Mainnet's chainId when constructing the Ethers provider, but this did not work. I suspect this failed because the provider we pass to ethers is not compliant with EIP 1193, as ethers doubtless expects it to be.

Instead the entire ethers provider is now reconstructed each time the network changes. This mirrors the approach we take in some other controllers.

Manual testing steps:

  • Change the current network to Ropsten
  • Restart the extension (e.g. on Chrome, navigate to chrome://extensions/ and click the "reload" circular arrow button on the MetaMask extension).
  • Unlock the extension, and observe that the current network is Ropsten
  • Switch to Mainnet
  • Navigate to swaps and attempt to get quotes
    • The "from" token may need to be a token that requires approval - I'm not totally sure.
  • Before this change, the attempt would always fail. After this change, it should succeed.

This is a continuation of #9726, which did not fix the problem
described.

If the initial network when the extension is started is something other
than Mainnet, the swaps controller will never successfully retrieve
swap quotes. This is because `ethers` will continue to communicate
with whichever network the provider was initially on.

We tried fixing this by hard-coding the `chainId` to Mainnet's
`chainId` when constructing the Ethers provider, but this did not work.
I suspect this failed because the `provider` we pass to `ethers` is not
compliant with EIP 1193, as `ethers` doubtless expects it to be.

Instead the entire `ethers` provider is now reconstructed each time the
network changes. This mirrors the approach we take in some other
controllers.
@Gudahtt Gudahtt requested a review from a team as a code owner October 28, 2020 17:56
@Gudahtt Gudahtt requested a review from brad-decker October 28, 2020 17:56
Co-authored-by: Thomas Huang <tmashuang@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@tmashuang tmashuang left a comment

Choose a reason for hiding this comment

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

Repro'd and LGTM. In my reproduction there is a slight delay of loading the my tokens in Swap from when immediately going to swap after network change, but don't know how we would optimize this.

Copy link
Copy Markdown
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [e47924d]
Page Load Metrics (494 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint308943168
domContentLoaded25974849214871
load26175149414871
domInteractive25974849214871

@Gudahtt Gudahtt merged commit 1294955 into develop Oct 28, 2020
@Gudahtt Gudahtt deleted the fix-fetching-of-swaps-when-initial-network-not-mainnet branch October 28, 2020 18:47
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants