-
Notifications
You must be signed in to change notification settings - Fork 75
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
Feat/ledger signing #1278
Feat/ledger signing #1278
Conversation
so that is can use the ledger-transport::Exchange trait for the transport which is async using this trait allows us to use the Zemu transport which can connect to the Speculos emulator
still need to handle errors properly
i needed to look at the js implementation again (hw-app-str) - they have a note that says to pass the `signatureBase` into the `signTransaction` fn. I had forgotten about this. The `signatureBase` fn is defined in js-stellar-base and is the value that should be sent to the network. it is TransactionSignaturePayload.toXDR(). Which is also in the signer train and i didn't notice it. 🙈
Currently don't support signing transaction, just its hash or any blob
Co-authored-by: Elizabeth Engelman <[email protected]>
Also clean up based on PR review
feat: add blob signer and simplify
03c891f
to
180757b
Compare
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.
Great work! Excited to finally get this out there!
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.
🎉
Looks super well organized and documented. Really glad to be able to ship this!
Co-authored-by: Chad Ostrowski <[email protected]>
impl ImageArgs for Args { | ||
fn into_iterator(self) -> Box<dyn Iterator<Item = String>> { | ||
let container_elf_path = format!("{DEFAULT_APP_PATH}/stellarNanosApp.elf"); | ||
let command_string = format!("/home/zondax/speculos/speculos.py --log-level speculos:DEBUG --color JADE_GREEN --display headless -s \"other base behind follow wet put glad muscle unlock sell income october\" -m nanos {container_elf_path}"); |
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.
Is the speculos file only used during testing? What's the significance of this hardcoded 12-word passphrase?
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.
speculos.rs
is only used in testing - like I mentioned in the other comment, I'm thinking I should create a tests
directory or something to put all test-only related files in there to make this more clear.
The hardcoded passphrase is the seed phrase for the account on the emulated device. It probably makes sense to pull this out into a constant.
I can make these changes in a follow-up PR!
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.
Ah got it. Yup makes sense.
We've kept tests in other repos in a tests/test module. Rust doesn't require it, but keeping tests in a separate module reduces rebuilds, supposedly, in large rust projects, which is why we use that pattern in other repos. This repo isn't big enough to see any benefit of that though, so not something we need to do. But moving stuff like this only used by tests makes sense.
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.
Just to close the loop, this PR makes this update: #1343
What
Add support for signing transactions on a Ledger device.
#1178
Why
Work toward key protection when submitting txns from the CLI. (#1255)
Known limitations
This does not implement the full
Stellar
signer trait yet. The stellar app deployed on the ledger device does not yet support signing soroban auth entries yet, so in the mean time we will only be able to do hash signing for soroban tx.Todo
txn sign
#1311