Custom wallet #106

robdmoore · 2023-08-27T05:51:12Z

Description

Added a new Custom wallet client that let's you delegate the wallet actions to the calling application; this let's you facilitate custom interactions like for instance the reason I built this - to facilitate a UI that allows you to guide users to do manual signing (think: Copper institutional wallet that doesn't support wallet connect, air-gapped computers, cloud HSMs, etc.).
- I could of course have implemented a ManualSigningWallet instead, but this felt like a missing extensibility point so I decided to go down this option, plus it means we don't need to expose a mechanism for the user to somehow supply their UI into use-wallet.
Made it so that metadata specified in an instance of a client wallet overrides metadata specified statically (so in this case you can override the custom wallet name).
Allowed building from windows by swapping rm -rf with rimraf
Added VS Code settings so prettier runs on save
Hiding clients that failed to initialise (felt like a logical thing to do, I noticed this "bug"(?) when I had an error initialising the custom wallet and was surprised it was still visible).

Checklist

Please, make sure to comply with the checklist below before expecting review

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary

I didn't add test or README yet since I wanted to first raise this for comment.

If you want to see an example of the initial wallet in action, check out https://testnet.xgov.algorand.foundation/. Note: at the moment the copy/paste is a bit intermittent, but I'm working to fix that.

…face

src/components/Example/Example.tsx

src/hooks/useWallet.ts

ehanoc · 2023-08-29T14:30:02Z

src/components/Example/Example.tsx

+
+      const forSigning = Buffer.from(
+        this.algosdk.encodeObj({
+          txn: unsignedTxn.get_obj_for_encoding()


I had a look at get_obj_for_encoding and it looks to me that it doesn't spit out the transaction in a format that is ready to be encoded + hashed + signed other than in goal (i.e cold vault system, hsm, hashicorp vault, raw EdDSA tool, etc)

get_obj_for_encoding returns the tx object with public keys instead of the addresses; which is what you would expect in a properly "ready-to-encode-and-sign" message. I think there's a goal bias here.

Wondering if anyone has ran into issues related to this and if we should rely algosdk.

In your implementation could you create the tx object using get_obj_for_encoding() then replace the public key with the address before encoding? algosdk.encodeAddress(new Uint8Array(Buffer.from(tx.snd, 'base64')))

Would there be a way to satisfy both the goal and non-goal use cases? If not, as an example I'd imagine goal is the easiest way to demo the provider. An alternative solution could be added to the documentation.

For non-goal signing... Maybe we could use algosdk#Transaction::bytesToSign() instead? Those can be "raw" signed by any system that can do EdDSA and produce a valid signature. We would then need non-goal instructions to add the signature. I can prep something.

What do you think @drichar ?

Sounds good! If you can provide an example that works for you, we can add a new Storybook story that demonstrates EdDSA signing.

Re: bytesToSign() though, doesn't that also rely on get_obj_for_encoding() and return the public key?

https://github.com/algorand/js-algorand-sdk/blob/develop/src/transaction.ts#L1116

Hey @drichar it does; but bytesToSign() knows how to perform all the rights tricks and conversions to spit out the raw data to perform EdDSA on.

I think i found the one flow to conquer it all :

Return the result of bytesToSign() instead of get_obj_for_encoding()

update the instructions to use the rawsend method on goal.

An alternative to goal could be the raw REST endpoint to post to any node described here: https://developer.algorand.org/docs/rest-apis/algod/#post-v2transactions

That should cover any situation where someone is signing with whatever (libsodium, tweenacl, HSM, Hashicorp Vault, Copper, anything that supports Ed25519 / EdDSA).

Thoughts?

CC: @robdmoore

Love it! Thanks for investigating. Will make this change when I come back to the PR.

Unfortunately, when trying this out it's not possible to get goal to rawsign, you can only rawsend, which can't work with this provider since we don't want goal to actually send the transaction :/

My suggestion would be that the wallet provider implementation that supports both has a tab/button to select between goal and non-goal signing and provides a different base64 to copy/paste.

drichar

Thank you @robdmoore for your contribution!

I tested the demo in Storybook using goal and it works as expected. I left a few comments; the requested change is to move the provider class to its own file. Everything else looks great.

Usually I would ask for the documentation to be updated when adding a new feature, but I still need to add KMD and MNEMONIC documentation anyway... I can also add a section for the CUSTOM provider. I might have you take a look at that when it's ready if you don't mind.

Edit: We're thrilled to see use-wallet on the XGov site! I'll add it to the "Used By" section of the README 🙌

src/components/Example/Example.tsx

src/hooks/useWallet.ts

drichar · 2023-08-29T23:06:48Z

Minor comment but I noticed you mentioned updating Prettier config to support auto-save, but I didn't see it in the changes.

Locally I have a .code-workspace file that includes auto-save settings, but it was added to the .gitignore way back when we started the library. That could be why it wasn't committed.

It probably does make sense to include it in version control though.

drichar · 2023-08-30T21:29:32Z

@robdmoore I think using CUSTOM for the provider ID is a little too vague. The XGov site uses the label "Manual" for this provider, and I think MANUAL as the provider ID might be closer to the mark. What do you think?

What are your thoughts @aorumbayev? (We were discussing this on Discord)

aorumbayev · 2023-08-30T21:36:01Z

@drichar yes perhaps Manual, Airgap or Offline might be a bit better?

robdmoore · 2023-08-31T06:38:16Z

Hey, busy week so it'll take me a few days to reply properly and adjust the PR, but I will do.

Re: custom vs manual etc. I specifically designed this wallet to allow for an app to develop a custom wallet as an extensibility hook. We could totally simplify it to instead be manual wallet only, but the way you display it ideally needs to be specific to the app e.g. look at what I did for voting app: https://github.com/algorandfoundation/nft_voting_tool/blob/main/src/xgov-dapp/src/features/wallet/ManualWallet.tsx

drichar · 2023-08-31T18:35:46Z

No worries @robdmoore! If it's easier I could move the TestManualProvider to a separate file. All things considered it's a pretty minor change, if you don't mind me adding commits to your PR.

Thanks for explaining your reasoning behind the CUSTOM provider ID. While a name like MANUAL or OFFLINE might make more sense with your example, the fact that you're not limited to something like that might make CUSTOM a better description after all. It could literally be anything, and we can make that clear in the documentation.

robdmoore · 2023-09-09T03:54:43Z

Just letting you know I haven't forgotten this and I will still be coming back and fixing it up ready for merge. Apologies for the delay thusfar.

robdmoore · 2023-09-25T15:48:25Z

Minor comment but I noticed you mentioned updating Prettier config to support auto-save, but I didn't see it in the changes.

Added that in

robdmoore · 2023-09-25T15:49:10Z

I'm not sure where the documentation should go for this? It doesn't feel like a standard use case so probably warrants its own section?

Also made the clipboard code more robust

robdmoore · 2023-09-25T15:54:55Z

I think this PR is good to go, the only thing left is the documentation.

Potentially we could write some automated tests too, but I suspect they may be of limited value in this case.

drichar · 2023-09-25T22:35:17Z

Thanks @robdmoore for making these changes! This looks great. I'll take a stab at the documentation. I agree it warrants its own section. If you don't mind, I might ask you or @aorumbayev to give it a read before shipping it.

FYI, just today I was thinking I may utilize the custom provider for something new we're cooking up. I really like the flexibility this feature introduces to the library. Appreciate your work on this.

drichar · 2023-09-25T22:42:53Z

I'm not sure where the documentation should go for this? It doesn't feel like a standard use case so probably warrants its own section?

If you'd like to handle the documentation, go for it! I probably won't get to this until tomorrow. Let me know if you would like to write it instead.

robdmoore · 2023-09-25T23:17:55Z

Feel free I’m happy to review!On 26 Sep 2023, at 6:43 am, Doug Richar ***@***.***> wrote: I'm not sure where the documentation should go for this? It doesn't feel like a standard use case so probably warrants its own section? If you'd like to handle the documentation, go for it! I probably won't get to this until tomorrow. Let me know if you would like to write it instead. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

robdmoore added 5 commits August 25, 2023 19:49

Initial implementation of custom client

44bcdf5

Allowed build from Windows

9b343af

Fixed problem with instance metadata not being displayed in the inter…

305ad35

…face

Fixing problem with unreliable clipboard copy

7ba14c6

Hiding clients that failed to initialise from the UI

01d091b

robdmoore commented Aug 27, 2023

View reviewed changes

src/components/Example/Example.tsx Outdated Show resolved Hide resolved

robdmoore commented Aug 27, 2023

View reviewed changes

src/hooks/useWallet.ts Show resolved Hide resolved

robdmoore commented Aug 27, 2023

View reviewed changes

src/hooks/useWallet.ts Show resolved Hide resolved

ehanoc reviewed Aug 29, 2023

View reviewed changes

drichar requested changes Aug 29, 2023

View reviewed changes

Added VS code auto-formatting

935ac45

Pull request feedback

be21af1

Also made the clipboard code more robust

robdmoore changed the title ~~[RFC] Custom wallet~~ Custom wallet Sep 25, 2023

drichar approved these changes Sep 25, 2023

View reviewed changes

drichar merged commit f1bb0f1 into TxnLab:main Oct 5, 2023
1 check passed

drichar mentioned this pull request Jun 5, 2024

feat: add Custom provider #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom wallet #106

Custom wallet #106

robdmoore commented Aug 27, 2023 •

edited

Loading

ehanoc Aug 29, 2023 •

edited

Loading

drichar Aug 29, 2023

drichar Aug 29, 2023

ehanoc Aug 30, 2023

drichar Aug 30, 2023

ehanoc Sep 8, 2023 •

edited

Loading

ehanoc Sep 8, 2023

robdmoore Sep 9, 2023

robdmoore Sep 25, 2023

drichar left a comment •

edited

Loading

drichar commented Aug 29, 2023

drichar commented Aug 30, 2023

aorumbayev commented Aug 30, 2023

robdmoore commented Aug 31, 2023

drichar commented Aug 31, 2023

robdmoore commented Sep 9, 2023

robdmoore commented Sep 25, 2023

robdmoore commented Sep 25, 2023

robdmoore commented Sep 25, 2023

drichar commented Sep 25, 2023 •

edited

Loading

drichar commented Sep 25, 2023

robdmoore commented Sep 25, 2023 via email

Custom wallet #106

Custom wallet #106

Conversation

robdmoore commented Aug 27, 2023 • edited Loading

Description

Checklist

ehanoc Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

drichar Aug 29, 2023

Choose a reason for hiding this comment

drichar Aug 29, 2023

Choose a reason for hiding this comment

ehanoc Aug 30, 2023

Choose a reason for hiding this comment

drichar Aug 30, 2023

Choose a reason for hiding this comment

ehanoc Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

ehanoc Sep 8, 2023

Choose a reason for hiding this comment

robdmoore Sep 9, 2023

Choose a reason for hiding this comment

robdmoore Sep 25, 2023

Choose a reason for hiding this comment

drichar left a comment • edited Loading

Choose a reason for hiding this comment

drichar commented Aug 29, 2023

drichar commented Aug 30, 2023

aorumbayev commented Aug 30, 2023

robdmoore commented Aug 31, 2023

drichar commented Aug 31, 2023

robdmoore commented Sep 9, 2023

robdmoore commented Sep 25, 2023

robdmoore commented Sep 25, 2023

robdmoore commented Sep 25, 2023

drichar commented Sep 25, 2023 • edited Loading

drichar commented Sep 25, 2023

robdmoore commented Sep 25, 2023 via email

robdmoore commented Aug 27, 2023 •

edited

Loading

ehanoc Aug 29, 2023 •

edited

Loading

ehanoc Sep 8, 2023 •

edited

Loading

drichar left a comment •

edited

Loading

drichar commented Sep 25, 2023 •

edited

Loading