Swaps support for local testnet by danjm · Pull Request #10658 · MetaMask/metamask-extension

danjm · 2021-03-16T10:17:18Z

This PR allows swaps to work on local test networks with chainId of 0x539. It also abstracts swaps logic dependent on chainId in a way that will allow us to add support for future networks simply by editing the shared/constants/swaps.js file.

Video demo of swaps working on a local test network:

swap-testnet-support.mp4

github-actions · 2021-03-16T10:17:35Z

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.

metamaskbot · 2021-03-16T10:30:41Z

Builds ready [daa8a2a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (755 ± 53 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	47	93	66	11	5
		domContentLoaded	577	926	754	110	53
		load	579	931	755	111	53
		domInteractive	577	926	753	110	53

brad-decker · 2021-03-16T14:09:53Z

app/scripts/controllers/transactions/index.js

          txMeta.txParams.from,
          txMeta.destinationTokenDecimals,
          approvalTxMeta,
+          chainId,


txMeta should now include the chainId for the transaction. I think this is safer than getting the current chainId if in the event that the network switches while this flow is in flight.

addressed in 0a89657

brad-decker · 2021-03-16T14:11:22Z

shared/constants/swaps.js

+
+const METASWAP_ETH_API_HOST = 'https://api.metaswap.codefi.network';
+
+const SWAPS_TESTNET_CHAIN_ID = '0x539';


what prevents a local ganache that doesn't fork mainnet or have the ability to interact with swaps from using this API?

hmmm.... good point... nothing prevents that

I think that's okay though... no risk of fund loss, and it is only developers using local ganache

brad-decker · 2021-03-16T14:20:10Z

ui/app/components/app/transaction-list/transaction-list.component.js

    return (
      txParams?.to === recipientAddress ||
-      (txParams?.to === SWAPS_CONTRACT_ADDRESS &&
+      (txParams?.to === SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[chainId] &&


[Q] It's been a while since I viewed this code, and regret not having added documentation to it when I touched it last. If I understand correctly this is to help filter out transactions that aren't to the specified token. So when we're on, let's say DAI's asset page, we only see transactions interacting with DAI's contract address and swaps's contract address as long as the swap interacted with DAI's contract address?

I was able to walk through that logic in the current develop branch without the addition of chainId. If my assumption is correct it might be worth documenting the filter logic so that we don't lose the intent of these filters as logic changes.

So when we're on, let's say DAI's asset page, we only see transactions interacting with DAI's contract address and swaps's contract address as long as the swap interacted with DAI's contract address?

yes, exactly

brad-decker · 2021-03-16T14:25:38Z

ui/app/ducks/swaps/swaps.js

    let swapsFeatureIsLive = false;
    try {
-      swapsFeatureIsLive = await fetchSwapsFeatureLiveness();
+      swapsFeatureIsLive = await fetchSwapsFeatureLiveness(chainId);


I just noticed that this method is in a utility file that exists in pages/swaps/swap.util but is also used in the background. Could we move these shared utility methods into shared/ ?

I have started this on a separate branch that I will PR after the present PR is merged https://github.com/MetaMask/metamask-extension/pull/new/swaps-utils-shared-directory

brad-decker · 2021-03-16T14:29:35Z

ui/app/ducks/swaps/swaps.js

 export const signAndSendTransactions = (history, metaMetricsEvent) => {
  return async (dispatch, getState) => {
+    const state = getState();
+    const chainId = getCurrentChainId(state);


[Q] would it be worth it to extract fetchSwapsFeatureLiveness into its own dispatch action / thunk so that anywhere we call it in the UI code it can be responsible for getting chainid itself? Seems like it'd cut down on repetition.

I have made this change on another branch that I will PR once this one is merged: https://github.com/MetaMask/metamask-extension/pull/new/fetchswapsfeatureliveness-thunk

brad-decker · 2021-03-16T14:35:35Z

ui/app/pages/swaps/build-quote/build-quote.js

  const conversionRate = useSelector(getConversionRate);
  const currentCurrency = useSelector(getCurrentCurrency);

+  const defaultTokenSymbol = SWAPS_CHAINID_DEFAULT_TOKEN_MAP[chainId].symbol;


[Q] Do you expect defaultTokenSymbol to be different than defaultSwapsToken.symbol here? Seems like getSwapsDefaultToken returns this same object?

No, I have addressed this as part of 04e7af6

that commit is dependent on b20c98f

brad-decker · 2021-03-16T21:03:55Z

@danjm the changes to tests will need to be applied onto the relocated versions that are now colocated with the code. Sorry for the inconvenience 🙏🏻

…lt swaps token

…sFilter

… comparisons in build-quote.js

danjm · 2021-03-17T07:23:53Z

@brad-decker This has been rebased and test related conflicts handled.

Note that there is one commit of updates that I made that are not directly from review: b20c98f "Create util method for comparison of token addresses/symbols to default swaps token"

metamaskbot · 2021-03-17T07:38:41Z

Builds ready [04e7af6]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (600 ± 15 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	71	53	8	4
		domContentLoaded	553	660	598	32	15
		load	554	661	600	32	15
		domInteractive	553	660	598	32	15

darkwing · 2021-03-17T14:36:35Z

app/scripts/controllers/swaps.js

+      // If the swap is from the selected chain's default token, subtract
+      // the sourceAmount from the total cost. Otherwise, the total fee
+      // is simply trade.value plus gas fees.
+      const ethFee = isSwapsDefaultTokenAddress(sourceToken, chainId)


nit: Do we want a more descriptive var name here? defaultTokenFee or transactionTokenFee? Just a thought. Naming is hard though.

This is a good point. There actually are a bunch of variables related to swaps with 'eth' in the name that need to be updated. I think "chainCurrency" is a good alternative to "eth". Because the changes are so extensive, I'd like to do this in a separate/future PR, to minimize chance of missing bugs. I have started the work on this branch https://github.com/MetaMask/metamask-extension/pull/new/eth-naming-updates-swaps-alt-nets

darkwing · 2021-03-17T14:48:28Z

app/scripts/controllers/transactions/index.js

          txMeta.txParams.from,
          txMeta.destinationTokenDecimals,
          approvalTxMeta,
+          txMeta.chainId,


Looking at the number of arguments going to this function, we're going to want to shift to passing an object sooon.

I have put this changed on another branch and will PR it once this PR is merged https://github.com/MetaMask/metamask-extension/pull/new/getSwapsTokensReceivedFromTxMeta-object-param

darkwing · 2021-03-17T15:08:14Z

ui/app/components/app/transaction-list/transaction-list.component.js

+) => {
  return ({ initialTransaction: { txParams } }) => {
    return (
      txParams?.to === recipientAddress ||


Should we do a check here for:

if (!txParams) { return ____ }

Might be shorter than a bunch of ?. optional chaining checks.

Given our lint rules, this would have to be over 3 lines, meanwhile two other lines just become shorter by a single character. So might be best to leave as is, unless adding the explicit if check and return is a readability improvement?

darkwing · 2021-03-17T15:15:59Z

ui/app/pages/swaps/build-quote/build-quote.js

    toToken;
-  const toTokenIsNotEth =
+  const toTokenIsNotDefault =
    selectedToToken?.address &&


I don't think we need this check because isSwapsDefaultTokenAddress inherently would return false

right, but line 137 is "**!**isSwapsDefaultTokenAddress(selectedToToken?.address, chainId)", so when selectedToToken?.address is a falsy value, toTokenIsNotDefault is true, because of the !

darkwing · 2021-03-17T15:21:24Z

I've made a few minor suggestions. Really well done, can't wait to use this!

danjm requested a review from a team as a code owner March 16, 2021 10:17

danjm requested a review from darkwing March 16, 2021 10:17

brad-decker suggested changes Mar 16, 2021

View reviewed changes

danjm added 5 commits March 17, 2021 03:40

Swaps support for local testnet

358eb22

Create util method for comparison of token addresses/symbols to defau…

b20c98f

…lt swaps token

Get chainId from txMeta in _trackSwapsMetrics of transaction controller

0a89657

Add comment to document purpose of getTransactionGroupRecipientAddres…

b7f2487

…sFilter

Use isSwapsDefaultTokenSymbol in place of repeated defaultTokenSymbol…

04e7af6

… comparisons in build-quote.js

danjm force-pushed the support-swaps-testnet branch from daa8a2a to 04e7af6 Compare March 17, 2021 07:22

brad-decker approved these changes Mar 17, 2021

View reviewed changes

darkwing reviewed Mar 17, 2021

View reviewed changes

danjm merged commit 480512d into develop Mar 18, 2021

danjm deleted the support-swaps-testnet branch March 18, 2021 10:20

github-actions bot locked and limited conversation to collaborators Mar 18, 2021


		const METASWAP_ETH_API_HOST = 'https://api.metaswap.codefi.network';

		const SWAPS_TESTNET_CHAIN_ID = '0x539';

Conversation

danjm commented Mar 16, 2021

Uh oh!

github-actions bot commented Mar 16, 2021

Uh oh!

metamaskbot commented Mar 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Mar 16, 2021

Uh oh!

danjm commented Mar 17, 2021

Uh oh!

metamaskbot commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkwing commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants