Ledger Live new API updates #780
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add `signWithdrawMessage` function to support the clear signing with Ledger Live. This function is optional so if the bitcoin provider supports the clear signing it should implement this method and this function will be used in the withdrawal flow.
We need to use the custom Acre module to support the clear signing.
We use new methods from the Ledger Live Wallet API and we need to set them in manifest file.
Adjust the Acre SDK to the new orangekit API.
We can sign the message with correct derivation path in Ledger Live.
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Expose the `BitcoinAddressHelper` function from the Orangekit package. We need this function in the dapp.
We want to port the Ledger Live Bitcoin provider implementation from orangekit to Acre as it contains many details specific to the Acre dapp only.
Move the Ledger Live connector and bitcoin provider implementation from the orangekit package to the dapp. Since there are many implementation details specified for Acre dapp, we decided to keep this implementation on Acre side.
We use `transactionSignAndBroadcast` function from the Acre custom module.
Make the `normalizeV` method private.
We can use `BitcoinNetwork` enum defined in Acre SDK.
We check above if the `this.#account` is undefined.
Pass correct network param to `AcreLedgerLiveBitcoinProvider` constructor.
Set the `readonly` variables at the beginning.
Extract the sign message shared code to a separate function.
Test function with supported and unsupported addresse types.
Display correct method name for test.
Let's extract this derivation path to a constant and use it in functions. We want to always operate on the zero address.
Adjust the code to the new Orangekit API.
Add the `bignumber.js` lib used by the Ledger Live packages. When ignoring the TS error we're losing the advantage of the type safety TS provides. We assume that the external library performs the conversion, but if the implementation in the Ledger library changes, we will only find out at runtime. Here we pass the satoshi value as a BigNumber object to the Ledger Live module.
nkuba
reviewed
Oct 30, 2024
The `satoshis` param is a `number` and there is no need to convert it to string when createing the `BigNumber` object.
Update `signMessage` and `signWithdrawMessage` docs. These functions return signature not hash of the signed message.
We can import SafeTransactionData from `./bitcoin` instead of `@orangekit/sdk`.
Use the defined type `BitcoinSignSafeTransactionMessageFn` from the `@orangekit/sdk` package.
Cover a case when the bitcoin provider implements `signWithdrawMessage` and that one that doesn't.
nkuba
reviewed
Oct 31, 2024
| @@ -29,6 +32,13 @@ export interface BitcoinProvider { | |||
|
|
|||
| /** | |||
| * Signs withdraw message. | |||
| * @param message Message to sign. | |||
There was a problem hiding this comment.
Let's add a comment that this function is optional, and if not implemented, we will fall back to the signMessage function.
dapp/src/utils/orangekit/ledger-live/tests/bitcoin-provider.test.ts
Outdated
Show resolved
Hide resolved
Mention in the docs that `signWithdrawMessage` function is optional and if it is not implemented the `signMessage` function will be used to sign the withdrawal message.
`LedgerLiveWalletApiBitcoinProviderOptions` -> `AcreLedgerLiveBitcoinProviderOptions` to match the provider name.
We should return the address at `0` index so we need to call the `getAdress` function from the bitcoin module. The address from the account object is always a fresh address and may be different from address at `0` index.
r-czajkowski
force-pushed
the
ledger-live-new-api-updates
branch
from
bb447b2
to
5de537c
Compare
nkuba
approved these changes
Oct 31, 2024
There was a problem hiding this comment.
I left one non-blocking comment, that can be addressed in another PR..
Comment on lines
+38
to
+40
| * @param data Withdrawal transaction data. @returns A signature for a given | ||
| * message, which proves that the owner of the account has agreed to the | ||
| * message content. |
There was a problem hiding this comment.
We're missing a line break before @returns.
There was a problem hiding this comment.
Fixed in be9fa83.
nkuba
added a commit
that referenced
this pull request
Nov 4, 2024
Depends on: #780 This PR fixes Ledger Live wallet connection bug. If the user rejects the login message signing and closes the modal, they should get back to the initial state of the not-connected account. Steps to recreate the bug: 1. Open the account connection. 2. Select a supported account. 3. Cancel login message signing and clse the connection modal. 4. The dApp looks like it is connected to an account, but after switching the account, it shows a "Something went wrong" error.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the implementation of the Ledger Live Bitcoin provider and connector from Orangekit to the Acre dapp. We decided to keep this implementation on the Acre side because there are many implementation details specified for Acre. Here we also use new functions from the new Ledger Live Wallet API.