-
Notifications
You must be signed in to change notification settings - Fork 7
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
add KeyManager stuff from old wallet sdk #98
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is filesystem access?Accesses the file system, and could potentially read sensitive data. If a package must read the file system, clarify what it will read and ensure it reads only what it claims to. If appropriate, packages can leave file system access to consumers and operate on data passed to it instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
import { ledgerHandler } from "./handlers/ledger"; | ||
import { plaintextKeyHandler } from "./handlers/plaintextKey"; | ||
// TODO - fix trezor errors | ||
// import { trezorHandler } from "./handlers/trezor"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the trezor code is throwing some errors so I'm commenting out for a next PR
@piyalbasu for this error:
Cannot find module 'stellar-sdk' from 'node_modules/@trezor/connect-plugin-stellar/lib/index.js'
Require stack:
node_modules/@trezor/connect-plugin-stellar/lib/index.js
src/walletSdk/KeyManager/handlers/trezor.ts
src/walletSdk/KeyManager/index.ts
src/index.ts
test/keyManager.test.ts
Because trezor has a peer dependency of stellar-sdk , instead of @stellar/stellar-sdk
That's something we need to ask them to change right? or do you know of a way to bypass that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vibrant is only using KeyType.plaintextKey
so we should be fine removing trezor unless Freighter is using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, I'll leave it commented out for now
looks like freighter is only using plaintextKey too atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply, yeah Freighter is only using plantextKey, so probably okay to drop Trezor support for now
I created this ticket to have Trezor update their deps: https://stellarorg.atlassian.net/browse/WAL-1316
some dev dependencies w/ vulnerabilities looks like, looking into fixing those |
hmm looks like there's a dependency alert for cross-fetch and node-fetch, both from the trezor-connect package maybe another thing to ping trezor about, wdyt @piyalbasu ? |
moving to drafts after convo: https://stellarfoundation.slack.com/archives/C03347FNAHK/p1707849301305449 |
Yeah, the The |
closing this PR in lieu of this one that's using yarn workspace |
ticket
largely just lifted from old wallets sdk