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

Keybase wallet plugin #2052

Merged
merged 11 commits into from Dec 5, 2018

Conversation

Projects
None yet
5 participants
@mcdallas
Copy link
Contributor

mcdallas commented Nov 29, 2018

Adds a new wallet communication adaptor to exchange slates via keybase chat.

grin wallet send -m keybase -d <recipient> <amt> for sending (only synchronously for now)
grin wallet listen -m keybase initialises a keybase listener

@mcdallas mcdallas force-pushed the mcdallas:keybase branch from 8b70cba to 6b1026a Nov 30, 2018

@quentinlesceller
Copy link
Member

quentinlesceller left a comment

Overall great ! just a minor comment.

Show resolved Hide resolved src/bin/grin.rs
@ignopeverell

This comment has been minimized.

Copy link
Member

ignopeverell commented Nov 30, 2018

So are we merging plug-ins in the wallet now? Isn't the point of a plug-in that it can be plugged-in (or not)? Or is this meant as a demo plug-in of sorts?

@yeastplume

This comment has been minimized.

Copy link
Member

yeastplume commented Nov 30, 2018

I hadn't intended this quite yet, the refactoring is done to allow plugins to exist, but the infrastructure bits to externalise them aren't. However, this is a good sample use-case and will help ensure the plug-in interface is actually useful, so not against having this as a demo plugin (that comes with 'experimental' warnings)

@mcdallas

This comment has been minimized.

Copy link
Contributor

mcdallas commented Nov 30, 2018

@ignopeverell my understanding from the meeting is that it was decided to have two "official" ones for http and keybase and the rest could be external or even seperate crates

@mcdallas

This comment has been minimized.

Copy link
Contributor

mcdallas commented Dec 2, 2018

I fixed some issues (thanks @mably and @quentinlesceller for helping me test it) and it seems to work fine. Only issue I am not 100% sure it was fixed is a crash when the keybase channel between the two parties was uninitialised. Will have to test with someone I haven't tested before to make sure.

@mcdallas mcdallas changed the title [WIP] Keybase wallet plugin Keybase wallet plugin Dec 2, 2018

@yeastplume

This comment has been minimized.

Copy link
Member

yeastplume commented Dec 3, 2018

Think it looks great and will be a nice little addition (especially now it's using the wallet API directly). Would definitely like to hear @ignopeverell's concerns as he doesn't seem convinced.

To be clear, I think this would be okay as a 'use at your own risk' example. There's an action following from this to externalise plugins somehow, so they (and any dependencies they might want to pull in) don't end up bloating grin's dependency tree.

Also @mcdallas, if we're okay to merge this would you mind documenting it's use somewhere in docs\usage\wallet.md?

@mably

This comment has been minimized.

Copy link

mably commented Dec 3, 2018

One thing I find a bit annoying is that, even if only the keybase listener is required to make a keybase tx, you still need to start the http listener if you want to have access to all the other wallet information like txs or outputs.

It could be interesting to be able to start multiple listeners in one single command.

@ignopeverell

This comment has been minimized.

Copy link
Member

ignopeverell commented Dec 3, 2018

@yeastplume I'm fine with merging this as long as we have an understanding that the purpose is to have a good couple of plugin examples. I would just object to start accumulating plugins under the grin wallet tree.

@yeastplume

This comment has been minimized.

Copy link
Member

yeastplume commented Dec 5, 2018

All good then, thanks for this @mcdallas

@yeastplume yeastplume merged commit f926131 into mimblewimble:master Dec 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment