Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

frontend: Migrate to use the new MetaMask API #339

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

manuelwedler
Copy link
Contributor

Fixes #338

MetaMask removed the injected window.web3 object recently, which we relied on. This broke our integration of MetaMask.

MetaMask should now be used through the window.ethereum API, which is specified by EIP 1193. This PR migrates the frontend to use this API to make the transaction request. The only place where we send a transaction from the frontend is for funding the user account.

This uses a small library (@metamask/detect-provider) to detect the rpc provider as recommended by MetaMask.

@manuelwedler manuelwedler requested a review from ezdac June 7, 2021 10:21
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #339 (8359d35) into develop (ca869af) will decrease coverage by 24.95%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #339       +/-   ##
============================================
- Coverage    89.57%   64.61%   -24.96%     
============================================
  Files           22       22               
  Lines         1775     1775               
============================================
- Hits          1590     1147      -443     
- Misses         185      628      +443     
Flag Coverage Δ
python-integration 53.07% <ø> (-30.82%) ⬇️
python-unit 40.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiden_installer/web.py 19.11% <0.00%> (-71.57%) ⬇️
raiden_installer/web_testnet.py 26.66% <0.00%> (-70.00%) ⬇️
raiden_installer/shared_handlers.py 32.27% <0.00%> (-64.15%) ⬇️
raiden_installer/transactions.py 46.93% <0.00%> (-48.98%) ⬇️
raiden_installer/utils.py 67.16% <0.00%> (-25.38%) ⬇️
raiden_installer/token_exchange.py 81.67% <0.00%> (-16.04%) ⬇️
raiden_installer/account.py 86.23% <0.00%> (-13.77%) ⬇️
raiden_installer/ethereum_rpc.py 77.89% <0.00%> (-13.69%) ⬇️
raiden_installer/network.py 77.55% <0.00%> (-8.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca869af...8359d35. Read the comment docs.

Copy link

@ezdac ezdac left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell. Added some questions, but most of them are out of curiosity.
Please ignore if the answer is obvious.
I believe the 1 missing error handling should be implemented though.

resources/static/js/account.js Outdated Show resolved Hide resolved
async function connectWeb3() {
let accounts = [];
try {
accounts = await provider.request({ method: 'eth_accounts' });
Copy link

@ezdac ezdac Jun 7, 2021

Choose a reason for hiding this comment

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

Do you even need this method, and is there a scenario that this will not return an empty list?
I undertstood it that way that the eth_requestAccounts should be generally used.
This is ok for the user, since there will only be a UI-prompt when access is requested the first time (?)

try {
    // Request account access if needed
    const accounts = await ethereum.send('eth_requestAccounts');
    // Accounts now exposed, use them
    ethereum.send('eth_sendTransaction', { from: accounts[0], /* ... */ })
} catch (error) {
    // User denied account access
}

(from EIP-1102)

I could be wrong - so I'm mainly just curious.

Copy link
Contributor Author

@manuelwedler manuelwedler Jun 10, 2021

Choose a reason for hiding this comment

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

The user may already have given the allowance to access the accounts, when the button is clicked. For example in case the button is clicked a second time after canceling the first transaction.

In my understanding, every call to eth_requestAccounts may trigger a user interface, depending on the RPC Provider. Since this is not the case with MetaMask, I think we can just use the call to eth_requestAccounts.

Copy link

Choose a reason for hiding this comment

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

Ok - I couldn't find anything about eth_accounts being unlocked by the eth_requestAccounts as well,
and the example code from the metamask docs was using eth_requestAccounts just before a transaction.

resources/static/js/account.js Show resolved Hide resolved
- Handle errors from requesting `eth_chainId`
- Remove `eth_accounts` request and only use `eth_requestAccounts`
@manuelwedler manuelwedler requested a review from ezdac June 10, 2021 09:18
Copy link

@ezdac ezdac left a comment

Choose a reason for hiding this comment

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

Looks good

@manuelwedler manuelwedler merged commit c66aae4 into raiden-network:develop Jun 10, 2021
@manuelwedler manuelwedler deleted the fix-metamask branch June 10, 2021 10:00
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.

MetaMask is not working anymore
2 participants