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

WIP: Trezor Integration #4296

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

memoriesadrift
Copy link

Opening this PR early as we talked about on Slack.
I'll leave questions in comments.

Copy link

socket-security bot commented Mar 4, 2024

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

Package New capabilities Transitives Size Publisher
npm/@babel/[email protected] environment 0 24.1 kB nicolo-ribaudo
npm/@babel/[email protected] None 0 63.8 kB nicolo-ribaudo
npm/@babel/[email protected] None 0 31.7 kB nicolo-ribaudo
npm/@babel/[email protected] None 0 49.1 kB nicolo-ribaudo
npm/@babel/[email protected] None 0 262 kB nicolo-ribaudo
npm/@babel/[email protected] environment 0 2.41 MB nicolo-ribaudo
npm/@emotion/[email protected] environment 0 130 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 10.2 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 4.29 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 58.6 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 30.8 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 7.05 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 7.38 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 18.3 kB emotion-release-bot
npm/@emotion/[email protected] environment 0 6.08 kB emotion-release-bot
npm/@floating-ui/[email protected] None 0 208 kB atomiks
npm/@floating-ui/[email protected] None 0 147 kB atomiks
npm/@floating-ui/[email protected] None 0 47.8 kB atomiks
npm/@floating-ui/[email protected] None 0 72.7 kB atomiks
npm/@mui/[email protected] None 0 2.05 kB siriwatknp
npm/@mui/[email protected] environment 0 795 kB mnajdova
npm/@mui/[email protected] environment 0 801 kB mnajdova
npm/@mui/[email protected] None 0 7.89 kB mnajdova
npm/@popperjs/[email protected] None 0 1.46 MB fezvrasta
npm/@types/[email protected] None 0 2.95 kB types
npm/@types/[email protected] None 0 6.71 kB types
npm/@types/[email protected] None 0 18.5 kB types
npm/@types/[email protected] None 0 431 kB types
npm/[email protected] None 0 9.37 kB sindresorhus
npm/[email protected] None 0 38 kB kentcdodds
npm/[email protected] None 0 6.33 kB sindresorhus
npm/[email protected] filesystem 0 92 kB d-fischer
npm/[email protected] environment 0 60.7 kB aleshaoleg
npm/[email protected] None 0 1.25 MB faddee
npm/[email protected] None 0 12.9 kB ljharb
npm/[email protected] None 0 127 kB monastic.panic
npm/[email protected] None 0 9.04 kB qix
npm/[email protected] None 0 2.69 kB jbnicolai
npm/[email protected] filesystem 0 5.27 kB jsdnxx
npm/[email protected] None 0 31.4 kB ljharb
npm/[email protected] None 0 7.7 kB ljharb
npm/[email protected] None 0 12 kB ljharb
npm/[email protected] None 0 20.6 kB ljharb
npm/[email protected] None 0 8.77 kB ljharb
npm/[email protected] None 0 4.71 kB rexxars
npm/[email protected] None 0 4.05 kB qix
npm/[email protected] None 0 30.2 kB ljharb
npm/[email protected] None 0 3.46 kB tuxsudo
npm/[email protected] None 0 15.1 kB lydell
npm/[email protected] None 0 10.4 kB isaacs
npm/[email protected] None 0 21.8 kB kof
npm/[email protected] None 0 120 kB kof
npm/[email protected] None 0 107 kB kof
npm/[email protected] environment 0 50.5 kB kof
npm/[email protected] None 0 10.8 kB kof
npm/[email protected] environment 0 109 kB kof
npm/[email protected] None 0 151 kB kof
npm/[email protected] environment, eval 0 470 kB kof
npm/[email protected] None 0 5.39 kB eventualbuddha
npm/[email protected] None 0 16.5 kB jdalton
npm/[email protected] None 0 1.41 MB bnjmnt4n
npm/[email protected] environment 0 5.81 kB zertosh
npm/[email protected] None 0 21.8 kB alexreardon
npm/[email protected] None 0 5.49 kB sindresorhus
npm/[email protected] None 0 97.2 kB ljharb
npm/[email protected] None 0 27 kB ljharb
npm/[email protected] None 0 26.5 kB ljharb
npm/[email protected] None 0 72.7 kB ljharb
npm/[email protected] None 0 3.92 kB sindresorhus
npm/[email protected] None 0 5.41 kB sindresorhus
npm/[email protected] None 0 4.51 kB jbgutierrez
npm/[email protected] filesystem 0 5.41 kB sindresorhus
npm/[email protected] environment 0 5.66 kB alexeyraspopov
npm/[email protected] environment 0 24 kB acdlite
npm/[email protected] environment 0 244 kB eps1lon
npm/[email protected] environment 0 291 kB gaearon
npm/[email protected] None 0 27.9 kB benjamn
npm/[email protected] environment, filesystem 0 145 kB ljharb
npm/[email protected] environment 0 93.4 kB gnoff
npm/[email protected] None 0 23.2 kB ljharb
npm/[email protected] None 0 134 kB andarist
npm/[email protected] None 0 9.18 kB ljharb
npm/[email protected] None 0 8.31 kB alexreardon
npm/[email protected] None 0 3.5 kB sindresorhus

View full report↗︎

@@ -2,5 +2,5 @@
. "$(dirname "$0")/_/husky.sh"

npx lint-staged
node scripts/sync-expo-deps.js --dry-run
#node scripts/sync-expo-deps.js --dry-run
Copy link
Author

Choose a reason for hiding this comment

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

This script references files I don't have upon cloning the repo and prevented me from commiting at all 🤔

Comment on lines +54 to +55
// FIXME: Fix ETH Trezor properly
new SolanaTrezorKeyringFactory()
Copy link
Author

Choose a reason for hiding this comment

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

How would you like to handle the absence of ETH integration for now?

Comment on lines 304 to 317
private async trezorImport(walletDescriptor: WalletDescriptor): Promise<{
publicKey: string;
name: string;
}> {
const blockchainKeyring = this.blockchains.get(walletDescriptor.blockchain);
const trezorKeyring = blockchainKeyring!.trezorKeyring!;
console.log(
"[DEBUG] UserKeyring: trezorImport: walletDescriptor:",
walletDescriptor,
blockchainKeyring,
trezorKeyring
);

throw new Error("Method not implemented.");
Copy link
Author

Choose a reason for hiding this comment

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

When are these import methods called (for ledger too)? I can't seem to get it to be called, even though it should be called on keyring creation.

Comment on lines 112 to 118
this.responseQueue
.filter((response) => response.portName === port.name)
.forEach((response) => {
console.log("[DEBUG] ToSecureUITransportSender: err 3", response);
const responseWithId = {
name: response.request.name,
id: response.request.id,
Copy link
Author

Choose a reason for hiding this comment

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

The TrezorConnect popup throws this error -- I don't understand your transport layer enough to find out what's going wrong. Funnily enough, the data is returned correctly, but the transport fails here and the response doesn't reach the ImportWallets component.

Object.values(derivationPaths)
);

setProgress(1);
Copy link
Author

Choose a reason for hiding this comment

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

What's the point of thesetProgress? is it ledger specific and can it be ommited?

Comment on lines 121 to 128
console.log("[DEBUG] [TREZOR] SelectWallets result", result);
currentRequest.respond({
walletDescriptors: result.map((descriptor) => ({
...descriptor,
type: BlockchainWalletDescriptorType.HARDWARE,
device: "trezor",
})),
});
Copy link
Author

Choose a reason for hiding this comment

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

This goes through, issues occur somewhere after the respond is called.

* @returns boolean indicating validity
*/
export function isLegalTrezorPath(path: string) {
const pathRegex = /^m\/44'\/501'(\/\d'){0,2}$/;
Copy link
Author

Choose a reason for hiding this comment

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

Trezor wasn't accepting some paths from backpack, this filter ensures that the paths are accepted, though it leads to derivation paths fetched going from 118 paths to 21.

Copy link

socket-security bot commented Mar 25, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant