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

feat(wallets): trezor wallet support #3020

Merged
merged 34 commits into from
Aug 15, 2023
Merged

feat(wallets): trezor wallet support #3020

merged 34 commits into from
Aug 15, 2023

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Aug 8, 2023 •
edited
Loading

Summary

Done:

  1. Added an icon to the wallet list for Trezor
  2. Fixed issues from previous QA: Wallet/trezor #2216 (comment)
  3. Added support to change networks. It doesn't work by default, requires some settings: https://forum.trezor.io/t/how-to-fix-error-forbidden-key-path/8642/2
  4. Added a selector for account index. The selector doesn't have design! Requires @fairlighteth input
  5. Additionally added displaying of error during approving tokens

Nuances:

  1. After page refresh it takes some time to initialize connection with Trezor again, so, you will see in the first ~3 seconds the disconnected UI state
  2. Network or account change requires reinitialization of Trezor connection, so, on every change you will get a request on your Trezor
  3. There is no gasPrice for Goerli in CowSwap. I've temporary hardcoded 40 GWEI
image image image image

@shoom3301 shoom3301 requested review from a team August 8, 2023 11:05
@shoom3301 shoom3301 self-assigned this Aug 8, 2023
@vercel
Copy link

vercel bot commented Aug 8, 2023 •
edited by github-actions bot
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2023 3:27pm

shoom3301
shoom3301 commented Aug 8, 2023
View reviewed changes
src/modules/account/containers/AccountDetails/HwAccountIndexSelector.tsx Outdated
margin: 10px 0;
`

// TODO: add styles
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fairlighteth 🙏

shoom3301
shoom3301 commented Aug 8, 2023
View reviewed changes
craco.config.js Outdated
@@ -92,6 +92,7 @@ module.exports = {
http: require.resolve('stream-http'),
https: require.resolve('https-browserify'),
crypto: require.resolve('crypto-browserify'),
zlib: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@trezor/connect-plugin-ethereum requires @metamask/eth-sig-util that requires zlib

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding @metamask/eth-sig-util

Does it need to be explicitly installed? Isn't it enough to have it installed by the @trezor package?

@socket-security
Copy link

socket-security bot commented Aug 8, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@trezor/connect-plugin-ethereum 9.0.1 environment +13 3.45 MB trezor-ci
@trezor/connect-web 9.0.11 None +53 26.8 MB trezor-ci
hdkey 2.1.0 None +0 15.3 kB ryanzim
node-stdlib-browser 1.2.0 None +30 1.08 MB niksy
@types/hdkey 2.0.1 None +0 5.04 kB types
@metamask/eth-sig-util 5.1.0 environment +12 3.44 MB metamaskbot
vite-plugin-node-polyfills 0.11.1 None +38 24.1 MB voraciousdev

shoom3301
shoom3301 commented Aug 8, 2023
View reviewed changes
src/common/containers/TradeApprove/TradeApproveButton.tsx Outdated

const handleApprove = useCallback(async () => {
if (shouldZeroApprove) {
await zeroApprove()
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before we didn't display errors from token approving

@shoom3301 shoom3301 added the Wallets Wallet related label Aug 8, 2023
@shoom3301 shoom3301 mentioned this pull request Aug 8, 2023
Closed
alfetopito
alfetopito reviewed Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Nice job!
I have a few comments.
Keep in mind I couldn't test it as I don't have a Trezor setup.

craco.config.js Outdated
@@ -92,6 +92,7 @@ module.exports = {
http: require.resolve('stream-http'),
https: require.resolve('https-browserify'),
crypto: require.resolve('crypto-browserify'),
zlib: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding @metamask/eth-sig-util

Does it need to be explicitly installed? Isn't it enough to have it installed by the @trezor package?

src/modules/wallet/api/state.ts Outdated Show resolved Hide resolved
src/modules/wallet/api/updaters/HwAccountIndexUpdater.tsx Outdated Show resolved Hide resolved
src/modules/wallet/api/utils/getHwAccount.ts Outdated Show resolved Hide resolved
src/modules/wallet/web3-react/connection/index.tsx Outdated Show resolved Hide resolved
src/modules/wallet/web3-react/connectors/TrezorConnector/index.ts Outdated Show resolved Hide resolved
src/modules/wallet/web3-react/connectors/TrezorConnector/sendTransactionHandler.ts Outdated Show resolved Hide resolved
src/modules/wallet/web3-react/connectors/TrezorConnector/sendTransactionHandler.ts Outdated Show resolved Hide resolved
src/modules/wallet/web3-react/hooks/useEagerlyConnect.ts Outdated Show resolved Hide resolved
@elena-zh elena-zh mentioned this pull request Aug 11, 2023
Closed
@elena-zh
Copy link

Hey @shoom3301 , works great!
Only one edge case that I should mention:

  1. Connect to Trezor
  2. Send a request to change a network
  3. Press on the Cancel button on the Trezor pop-up
  4. Try running a transaction

AR: the TX will fail. User needs to refresh a page in order to proceed

ER: disconnect a user when press son Cancel? The app appears to be disconnected when I refresh the page.

Anyways, this issue can be fixed in another PR. I opened #3044 issue for this.

alfetopito
alfetopito reviewed Aug 14, 2023
View reviewed changes
apps/cowswap-frontend/src/modules/tokens/hooks/useCurrencyBalance.ts
Comment on lines -48 to +51
memo[address] = CurrencyAmount.fromRawAmount(nativeOnChain(chainId), JSBI.BigInt(value.toString()))
memo[lowerCaseAddress ? address.toLowerCase() : address] = CurrencyAmount.fromRawAmount(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not make all addresses lowercase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about other cases, don't want to break smth

@shoom3301 shoom3301 merged commit 40d1d22 into develop Aug 15, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2023
@alfetopito alfetopito deleted the feat/trezor-wallet-support branch August 15, 2023 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfetopito alfetopito alfetopito approved these changes

@elena-zh elena-zh elena-zh approved these changes

Assignees

@shoom3301 shoom3301

Labels
Wallets Wallet related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shoom3301 @elena-zh @alfetopito