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

lnrpc: add new WalletKit sub-RPC server #2093

Merged
merged 7 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@Roasbeef
Copy link
Member

commented Oct 25, 2018

In this PR, we add a new sub-RPC server to the existing set of gRPC
servers. This new sub-RPC server is the WalletKit. It's a utility
toolkit that contains method which allow clients to perform common
interactions with a wallet such as getting a new address, or sending a
transaction. It also includes some supplementary actions such as fee
estimation.

One thing to note in the RPC file is that we import the existing
signer.proto file in order to get at some existing proto definitions
which are useful in our use case.

Depends on #2081.

The number of satoshis per kilo weight that should be used when crafting
this transaction.
*/
int64 sat_per_kw = 1;

This comment has been minimized.

Copy link
@joostjager

joostjager Nov 5, 2018

Collaborator

In addition to this, we need a maximum total fee as well. We don't know how many inputs are needed, so the fee can potentially become (too) large.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 1, 2018

Author Member

So a max for the request, or a max in the response? Once you know the fee rate, you can use the weight estimator to compute what the final fee will be.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 3, 2018

Collaborator

I was thinking a max for the request. How can I use the weight estimator if I don't know how many inputs are needed?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 4, 2018

Author Member

Ah OK, IIRC there're two types of fee estimates we care about:

  1. What should the fee be for a transaction I custom craft myself
  2. What's an estimate for the transaction fee if I instruct the wallet to create a certain number of outputs

This PR as is gives us #1, for #2 there's #1228.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 4, 2018

Collaborator

I am suggesting 3. Set a hard cap on the actually paid tx fee if I instruct the wallet to create a certain nof outputs.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 5, 2018

Author Member

Gotcha, will make an issue to track this as this can be added in a new PR (since it also requires wallet modifications).

attempt to re-broadcast the transaction on start up, until it enters the
chain.
*/
rpc PublishTransaction(Transaction) returns (PublishResponse);

This comment has been minimized.

Copy link
@joostjager

joostjager Nov 5, 2018

Collaborator

How is this going to work with the rebroadcaster? It needs sign descriptors too.

This comment has been minimized.

Copy link
@joostjager

joostjager Nov 9, 2018

Collaborator

Suggestion: instead of PublishTransaction, this could be SendInputs which takes sign descriptors.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 1, 2018

Author Member

So just to check in again, I think we're find with PublishTransaction and may follow up with the two-stage craft+sign process?

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 3, 2018

Collaborator

Yes indeed, we leave it for a follow up.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2018

Need a way to persist the SendOutputs tx before it is published, to make sure we never accidentally pay to the same output again.

@Roasbeef Roasbeef force-pushed the Roasbeef:walletkit-service branch 2 times, most recently from b505b31 to 3c545bd Dec 1, 2018

The number of satoshis per kilo weight that should be used when crafting
this transaction.
*/
int64 sat_per_kw = 1;

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 3, 2018

Collaborator

I was thinking a max for the request. How can I use the weight estimator if I don't know how many inputs are needed?

attempt to re-broadcast the transaction on start up, until it enters the
chain.
*/
rpc PublishTransaction(Transaction) returns (PublishResponse);

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 3, 2018

Collaborator

Yes indeed, we leave it for a follow up.

KeyLoc: &signrpc.KeyLocator{
KeyFamily: int32(keyDesc.Family),
KeyIndex: int32(keyDesc.Index),
},

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 3, 2018

Collaborator

Pubkey bytes need to be returned? It looks like it currently just returns the request values.

switch {
case config.MacService == nil:
return nil, nil, fmt.Errorf("MacService must be set to " +
"create WalletKit RPC server")

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 4, 2018

Collaborator

It seems walletkit cannot be used with nomacaroons

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 5, 2018

Author Member

Fixed!

@Roasbeef Roasbeef force-pushed the Roasbeef:walletkit-service branch from 7963427 to 9c23d2c Dec 5, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

Addressed all lingering comments, PTAL!


// The default amount of logging is none.
func init() {
UseLogger(build.NewSubLogger("CRTR", nil))

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 5, 2018

Collaborator

needs different log tag

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 5, 2018

Author Member

Fixed!

// all our methods are routed properly.
RegisterWalletKitServer(grpcServer, w)

log.Debugf("WalletKit RPC server successfully register with " +

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 5, 2018

Collaborator

nit: register -> registered

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 5, 2018

Author Member

Fixed!

Show resolved Hide resolved subrpcserver_config.go

@Roasbeef Roasbeef force-pushed the Roasbeef:walletkit-service branch from 9c23d2c to 60b597c Dec 5, 2018

@cfromknecht
Copy link
Collaborator

left a comment

couple last things from a prior review that weren't addressed

// to execute common wallet operations. This includes requesting new addresses,
// keys (for contracts!), and publishing transactions.
type WalletKit struct {
started int32 // To be used atomically.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 6, 2018

Collaborator

started, stopped, and quit don't do anything, they can be removed

// NOTE: This is part of the lnrpc.SubServer interface.
func (w *WalletKit) Start() error {
if atomic.AddInt32(&w.started, 1) != 1 {
return nil

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 6, 2018

Collaborator

Start() can just return nil

// NOTE: This is part of the lnrpc.SubServer interface.
func (w *WalletKit) Stop() error {
if atomic.AddInt32(&w.shutdown, 1) != 1 {
return nil

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 6, 2018

Collaborator

Stop can just return nil

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

Didn't you want to add signer permission to the admin macaroon in this pr too?

// check to see if we need to create it or not.
macFilePath := cfg.WalletKitMacPath
if !fileExists(macFilePath) && cfg.MacService != nil {
log.Infof("Making macaroons for WaleltKit RPC Server at: %v",

This comment has been minimized.

Copy link
@wpaulino

wpaulino Dec 6, 2018

Collaborator
Suggested change
log.Infof("Making macaroons for WaleltKit RPC Server at: %v",
log.Infof("Baking macaroons for WalletKit RPC Server at: %v",

😛

Roasbeef added some commits Oct 25, 2018

lnrpc/walletrpc: add new sub-RPC server, the WalletKit
In this commit, we add a new sub-RPC server to the existing set of gRPC
servers. This new sub-RPC server is the WalletKit. It's a utility
toolkit that contains method which allow clients to perform common
interactions with a wallet such as getting a new address, or sending a
transaction. It also includes some supplementary actions such as fee
estimation.

One thing to note in the RPC file is that we _import_ the existing
signer.proto file in order to get at some existing proto definitions
which are useful in our use case.
lnrpc/walletrpc: implement the WalletKitServer gRPC service
In this commit, we implement the newly defiend WalletKitServer gRPC
service. We use the same template w.r.t build tags as the existing
signrpc service.
rpc: extend the admin macaroon with signer capabilities
In this commit, we extend the admin macaroon with signer capabilities in
order to allow it to be used with the new signer sub-server. As a
result, users will need to clear out their old macaroons in order to
have the new improved admin macaroon generated. In the future, we may
want to restructure the way the admin macaroon functions: rather than
white listing each of these entities and operations, we can instead add
a catch all capability. This capability will instead allow access to any
call, as each new call would be modified to permit this capabilities and
no others.

@Roasbeef Roasbeef force-pushed the Roasbeef:walletkit-service branch from 60b597c to 363b992 Dec 7, 2018

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2018

Final comments addressed!

@cfromknecht
Copy link
Collaborator

left a comment

LGTM 🎉

@Roasbeef Roasbeef merged commit eb16427 into lightningnetwork:master Dec 7, 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
You can’t perform that action at this time.