Adding ability to signMessage with Ledger adapter #712
Conversation
🦋 Changeset detectedLatest commit: 3d84674 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@elbywan any possibility of getting the new Solana app released with the off-chain signing?Thank you. |
|
Hey @martincik, sorry but I am not working in the Ledger firmware team and my work is not related to the nano apps at all. |
| /** @internal */ | ||
| export async function signMessage(transport: Transport, message: Buffer, derivationPath: Buffer): Promise<Buffer> { | ||
| const paths = Buffer.alloc(1); | ||
| paths.writeUInt8(1, 0); | ||
|
|
||
| const data = Buffer.concat([paths, derivationPath, message]); | ||
|
|
||
| return await send(transport, INS_SIGN_MESSAGE_OFFCHAIN, P1_CONFIRM, data); | ||
| } |
There was a problem hiding this comment.
Thanks, this looks great. I think we'll need to add some things to the adapter/utils to help apps prepare the right message format to sign, but this is a good start.
|
@jordansexton Thanks for the review and feedback. I've applied all the input and reverted all the new derivative path codes. Feel free to let me know if there's anything else I can do to move it forward. |
|
This looks good. I think we need to wait to merge until the Ledger app actually supports the full message preamble: https://edge.docs.solana.com/proposals/off-chain-message-signing#message-preamble The current (1.3.1) release does support But the addition to the Ledger app was done before the spec was finalized, and some of the message headers have changed. |
|
pinging you on this @jordansexton, when do you expect to merge this? |
|
hey @jordansexton , are there any plans to merge this anytime soon? |
|
This is still in development for the Ledger app. While the app was updated to include message signing support, it doesn't implement the final version of the Solana offline signing spec yet. Apps shouldn't start to use until it's implemented correctly (the message preamble has changed, so the input and signatures won't match). I'll merge this when it's released. |
|
so ledger is still working on this I assume? |
hey jordan was there any update on this? |
|
Hey any update on this? |
|
Hey, I know it was mentioned that the ledger app still needs updating before this can be merged. But it is my understanding that each of the apps on the ledger are not developed by ledger itself but by each of the blockchain providers? If that's the case, any chance you can find who is working there and see if that is even been worked on? This is a request that has been around since 2021... I wonder if there's pushback for some reason and just will never be done... Thanks! |
|
Hm, is that release of the app not done yet? Their versioning is a bit of a weird to me but it seems like the pr in question has been merged and released? LedgerHQ/app-solana#37 Unless I'm getting something wrong lol |
Looks like not everything is done yet. Check this previous answer by @jordaaash #712 (comment) It baffles my mind how this is still an issue after 3 years. I totally understand some people not even wanting this because it's not decentralizes to use web2 auth system with a web3 sign. Or because it's not good to have people using their ledgers all the time to sign auth messages. (i've heard both of those in the past). But the reality is that people still do it resorting to all sorts of workarounds which are even worse from a security stand point (disabling blind signing and signing random transactions that don't end up being sent on chain). That's an awful habit to put users into, and probably how they end up being drained. I hope the different teams needed to get this through can get together and try to push this forward "soonish" 🙏 |
For this patch to work, we would need a new Solana App released with supporting off-chain message signing. The code is done here: LedgerHQ/app-solana#37 not yet been released.