Add Trezor wallet adapter #839
Conversation
gabrielKerekes
force-pushed
the
add-trezor-wallet-adapter
branch
from
066b667
to
dbbb7d5
Compare
|
The The build also fails because the Solana functions are not supported by the current version of |
|
Thanks! Excited for this release. I can't merge it if the build is failing though. I'm also not inclined to merge before it's usable, or at least easily testable. |
|
Sure, makes sense. I will update the PR when a beta version of Maybe you could at this point just take a look at the code if there aren't some obvious things which would need to be changed? 🙏 |
|
👋 The release of Solana support on Trezor has been moved to December 15th (sorry for not writing here earlier). We've put together a doc which should help with the setup required to test the PR before December 15th. You can find the doc here. Do you think you could give it a try to test the adapter? It'd be great if we could release this on December 15th or 16th - or ideally earlier but I think a pre-release isn't really possible/feasible until the Trezor release? There might be a beta release of Trezor Connect before December 15th. Maybe this could be released with the beta Connect version before December 15th and then re-released with official Connect release after December 15th (e.g. on December 16th). What do you think? The reason it'd be nice to release before December 15th is that dApps could potentially prepare for the release beforehand and release Trezor support sooner. |
|
|
||
| this._connecting = true; | ||
|
|
||
| const connectUrl = this._connectUrl && this._connectUrl + (this._connectUrl.endsWith('/') ? '' : '/'); |
There was a problem hiding this comment.
Can we do this in the constructor?
| manifest: { | ||
| email: 'maintainers@solana.foundation', | ||
| appUrl: 'https://github.com/solana-labs/wallet-adapter', | ||
| }, |
There was a problem hiding this comment.
Some info can be found here -> https://docs.trezor.io/trezor-suite/packages/connect/index.html#how-to-use
Mainly
This provides us with the ability to reach you in case of any required maintenance.
There was a problem hiding this comment.
Okay, then please update these to your own email and URL. We don't provide support for anything except for bugs with the adapter itself.
|
|
||
| const connectUrl = this._connectUrl && this._connectUrl + (this._connectUrl.endsWith('/') ? '' : '/'); | ||
|
|
||
| await TrezorConnect.init({ |
There was a problem hiding this comment.
This dependency will need to be async imported. Check out how some of the other adapters that load external libraries do it.
There was a problem hiding this comment.
The // @ts-ignore above the this._wallet assignment is needed because apparently the Trezor Connect types don't match the exports - something weird is going since we need to do basically do TrezorConnect.default.default. I'll notify the Connect guys regarding this, but for now, this should be good enough.
| @@ -0,0 +1,169 @@ | |||
| import type { WalletName } from '@solana/wallet-adapter-base'; | |||
| import TrezorConnect, { DEVICE, DEVICE_EVENT } from '@trezor/connect-web'; | |||
There was a problem hiding this comment.
These will need to be imported as types only. External SDKs must be loaded async on connect.
| TrezorConnect.on(DEVICE_EVENT, (event: any) => { | ||
| if (event.type === DEVICE.DISCONNECT) { | ||
| this._disconnected(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Since these look to be just strings, we can hardcode them.
| } | ||
| }); | ||
|
|
||
| this._publicKey = new PublicKey(Buffer.from(publicKey, 'hex')); |
There was a problem hiding this comment.
Can you get your API to return a Uint8Array here? Solana public keys never use hex strings, and I'd prefer not to add the Buffer dependency here, we try to avoid it wherever possible. It's already been eliminated from the new version of web3.js we're moving to.
There was a problem hiding this comment.
Can you get your API to return a Uint8Array here?
Sadly I don't think so. TrezorConnect accepts and returns bytes as hex for other blockchains as well. Also the Connect implementation has already been approved so I'd rather not introduce changes at this point - since we're very close to the release.
For now I'd suggest no action since I see the Solana SDK still returns a Buffer for transaction.message.serialize() and transaction.serializeMessage() and transaction.addSignature takes a Buffer as parameter.
What version of web3 will you be moving to? 2.0.0?
When Solana SDK no longer works with Buffer, I'd suggest we introduce local utils for hex <-> Uint8Array conversion and that would be all that is required.
What do you think?
There was a problem hiding this comment.
Okay, I think that's fine for now. I just don't want to end up importing a second copy of Buffer. I'll take a look at how this is getting built.
There was a problem hiding this comment.
(I resolved the other two comments I made, I'll leave this one pending for now, but I don't think there's any action you need to take on it.)
|
In cbb14e7 I've updated Connect to a version with Solana support as it's been released yesterday 😉 However I don't think we want the I'll try to figure out if there is a simple way for you to test this end to end now. |
If you're on Apple Sillicon I can't help you right now (because ARM Trezor firmware builds are failing), the only way to test is by building the Trezor emulator yourself - as mentioned in this doc. If you're on Linux you can use trezor-usr-env. It should only be required to clone the repo and run Or well, we can wait until Wednesday (Dec 13 2023) when the FW should be officially released. |
|
👋 So Solana support on Trezor has officially been released. Trezor devices can be updated via Trezor Suite. For testing you can still build the emulator and test against that. Please let me know what actions are required to get this merged and released or how I could help to achieve that 🙏 |
|
Thanks! Catching up on this now. |
|
@gabrielKerekes I've pushed an update to #876 because this PR's branch doesn't allow commits by maintainers. However, the issue that's being caused by Because this is a transitive dependency of Looking at the dependencies for that project: "devDependencies": {
"@trezor/e2e-utils": "1.0.0",
"@trezor/type-utils": "1.0.2",
"fs-extra": "^11.1.1",
"html-webpack-plugin": "^5.5.3",
"jest": "^26.6.3",
"rimraf": "^5.0.5",
"tiny-worker": "^2.3.0",
"tsx": "^3.14.0",
"typescript": "5.3.2",
"webpack": "^5.89.0",
"webpack-cli": "^5.1.4",
"webpack-dev-server": "^4.15.1",
"worker-loader": "^3.0.8"
},
"dependencies": {
"@solana/buffer-layout": "^4.0.1",
"@solana/web3.js": "^1.87.6",
"@trezor/blockchain-link-types": "1.0.8",
"@trezor/blockchain-link-utils": "1.0.9",
"@trezor/utils": "9.0.15",
"@trezor/utxo-lib": "2.0.1",
"@types/web": "^0.0.119",
"bignumber.js": "^9.1.1",
"events": "^3.3.0",
"ripple-lib": "^1.10.1",
"socks-proxy-agent": "6.1.1",
"ws": "7.5.9"
}I can see that https://www.npmjs.com/package/@types/web says
I can't see why this is required and I think it may be superfluous. What is it used for in this dependency, and can it be removed? |
|
(I'm responding in this PR since the original discussion is here) Thanks for the updates.
I'll notify Trezor devs regarding this.
Seems to work just fine 👍 |
* Add Trezor wallet adapter * Handle `connectUrl` only in the constructor * Switch to async import for TrezorConnect * Change Connect manifest email * Update @trezor/connect-web to a version with Solana support * refactor * add trezor to root tsconfig * override @types/web with typescript --------- Co-authored-by: gabrielkerekes <gabriel.kerekes@vacuumlabs.com>
|
Merged #876 which incorporates these commits. Thanks! |
|
Released via #878 |
This PR adds Trezor HW wallet adapter. Trezor Solana support is planned to be released on Nov 15 2023. Ideally we would have this PR merged beforehand so dApps could potentially prepare for the Trezor release and already be integrated on Nov 15.
At this point the current
@trezor/connect-webpackage (without Solana support) is included in@solana/wallet-adapter-trezor- this will need to be updated when a beta version of@trezor/connect-webwith Solana support is released - expected soon after Nov 1 and before Nov 15 and then updated again when the official version of@trezor/connect-webwith Solana support is released.Unofficial firmware builds should also be available between Nov 1 and Nov 15.
How to test
Testing the Trezor adapter is a bit difficult since none of the Trezor components (Trezor Connect and Trezor firmware) have yet been released and thus both Connect and FW have to be setup locally and the local
@trezor/connect-webhas to be linked into this proejct. I'm open to scheduling a demo call to demonstrate the functionality on my local setup. Or we can provide detailed steps on how to locally setup, build and run both Connect and Trezor FW.