Skip to content
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: add Ledger wallet integration #53

Open
wants to merge 12 commits into
base: development
Choose a base branch
from

Conversation

CDDelta
Copy link

@CDDelta CDDelta commented Nov 18, 2021

This PR adds support for connecting ArConnect to a Ledger with an Arweave wallet.

Note:

  • Multiple Ledger wallets can be added but only one may be active (connected to the browser) at once.
  • If the active wallet in ArConnect does not match the one on the connected Ledger an error is thrown.
  • Config exports exclude Ledger wallet records.
  • Due to limitations with the Ledger x Arweave API, encryption/decryption and custom signature requests resolve with an "Action not supported" error when called.
  • Transaction signing requests to a Ledger will error if provided with custom signature options.

@CDDelta
Copy link
Author

CDDelta commented Nov 19, 2021

Blocked on some typings issue with the Ledger SDK. We could potentially release with our own fork release of the SDK if needed.

@CDDelta CDDelta marked this pull request as ready for review November 19, 2021 08:28
@CDDelta CDDelta marked this pull request as draft November 23, 2021 23:44
@martonlederer
Copy link
Member

Is this ready to merge or should I merge the other one? I believe that also has the ledger stuff

@CDDelta
Copy link
Author

CDDelta commented Dec 5, 2021

Both are not ready yet, was pending a release of the Ledger SDK from Zondax's side which just got updated, will get on this tomorrow. When the time comes this will have to be merged first.

@martonlederer
Copy link
Member

Alright, thank you! Make sure to tag me when it's time

@CDDelta
Copy link
Author

CDDelta commented Dec 6, 2021

@martonlederer this PR is ready for your review. It conflicts with a change on development but I'm not sure which way to resolve it. What was the reasoning behind using getUploader() as opposed to post()?

@CDDelta CDDelta marked this pull request as ready for review December 6, 2021 05:34
@martonlederer
Copy link
Member

@CDDelta I noticed that the network is more likely to drop txs when it's using "post". It even worked better for me, than fee multiplication

@CDDelta
Copy link
Author

CDDelta commented Dec 6, 2021

@CDDelta I noticed that the network is more likely to drop txs when it's using "post". It even worked better for me, than fee multiplication

Interesting, resolved the merge conflicts with that and added a comment for it.

@martonlederer
Copy link
Member

Thanks! No idea why that happens...

@heiningair
Copy link

Is this feature request still in the making? If not is there an estimate when this could work, since ledger nano s already integrates arweave?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants