feat: add support for Turnkey server wallets #137
Conversation
✅ Deploy Preview for verbs-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![wiz-inc-a178a98b5d[bot]](https://avatars.githubusercontent.com/u/58791460?s=60&v=4){kind=small}
There was a problem hiding this comment.
More Details
Vulnerabilities [axios:1.9.0]
| Name | Severity | Source | Fixed version | CVSS score | CVSS exploitability score | Has public exploit | Has CISA KEV exploit |
|---|---|---|---|---|---|---|---|
| CVE-2025-58754 |  |
GHSA-4hjh-wcwx-xvwj | 1.12.0 | 7.5 | 3.9 | false | false |
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
There was a problem hiding this comment.
#wiz_ignore this vulnerability is related to a peer dependency which is only installed in the dev build of this library
There was a problem hiding this comment.
The finding has been successfully ignored for these checks.
If you'd like to ignore this finding in all future scans, you can add a matching exception in the .wiz file.
Learn more
There was a problem hiding this comment.
More Details
Vulnerabilities [bigint-buffer:1.1.5]
| Name | Severity | Source | Fixed version | CVSS score | CVSS exploitability score | Has public exploit | Has CISA KEV exploit |
|---|---|---|---|---|---|---|---|
| CVE-2025-3194 | |
GHSA-3gc7-fjrx-p6mg | - | 7.7 | 3.9 | false | false |
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
There was a problem hiding this comment.
#wiz_ignore this vulnerability would cause the app server to potentially crash if exploited. the app server is only being used for a demo application and a crash would not impact any production services.
There was a problem hiding this comment.
The finding has been successfully ignored for these checks.
If you'd like to ignore this finding in all future scans, you can add a matching exception in the .wiz file.
Learn more
tremarkley
force-pushed
the
harry/node_turnkey
branch
3 times, most recently
from
a09ba09 to
dcdfe34
Compare
tremarkley
force-pushed
the
harry/node_turnkey
branch
from
dcdfe34 to
b1cb30d
Compare
tremarkley
changed the title
tremarkley
changed the title
| * @returns Promise resolving to a Verbs-compatible wallet instance | ||
| */ | ||
| async toVerbsWallet( | ||
| params: TurnkeyHostedWalletToVerbsWalletOptions, |
There was a problem hiding this comment.
I've been avoiding using this params attr, but I think it actually makes sense. I'll probably switch LendProvider functions to use it.
There was a problem hiding this comment.
yeah, I'm in general a fan of using param types, I find it more readable and ergonomic
| /** | ||
| * Turnkey organization ID that owns the signing key | ||
| */ | ||
| private readonly organizationId: string |
There was a problem hiding this comment.
❓ Will this ever change from wallet to wallet? If not, should it live at an abstraction above the wallet instance, like the VerbsConfig?
There was a problem hiding this comment.
yeah, so looking at the turnkey docs, it is possible that all wallets could be under the same organizationId, but they also have support for each wallet being in its own sub-organization, which would mean a different organizationId per wallet. Here is more info: https://docs.turnkey.com/embedded-wallets/sub-organizations-as-wallets.
Given this I think it makes sense to abstract this at the wallet-level for now and we can always pull it into a higher level later if we find this is a big point of friction, but at least this design supports both use-cases (ie wallets as sub-organizations or all wallets under same organization)
| }) { | ||
| const { chainManager, client, organizationId, signWith, ethereumAddress } = | ||
| params | ||
| super(chainManager) |
There was a problem hiding this comment.
🤔 I know this isn't new to this class, but I've wondered if the ChainManager needs to be passed through every instance of everything, or if it can live at an abstraction above? Thinking out loud, I'm not sure what that would look like, but like supported assets and supported networks, it's not something that will change from wallet to wallet.
There was a problem hiding this comment.
even though ChainManager doesnt change from wallet to wallet, it does change for each Verbs instance and the wallet needs a way to access it. Right now we've opted for accessing it through dependency injection, which has it's tradeoffs, but the only other way we could potentially access it is through a global variable, which might make things more difficult to test and harder to trace. Let me know if there is another way you were thinking though or if you think a global variable would be preferred here
There was a problem hiding this comment.
Having thought through it a bit more, I think this is totally fine. Global state gets dangerous and passing around a single dependency is very manageable.
I’m imaging a future where additional context providers are added to verbs that allow the dev to overwrite functionality like:
ChainExplorerProvider: overwrite all receipt urls with blockscout or etherscanTransactionSimulationProvider: overwrite all wallet.action.quote functions with tenderly or alchemy simulations
If we get to that world, we might pull these dependencies into a shared context provider registry and pass that single object around, but that's well into the future.
There was a problem hiding this comment.
makes sense! if we get to a point where providers are coupled, I could see combining them making sense
Closes #139
Description
Add
TurnKeyWalletclass for supporting Turnkey server wallets