Closed bucko13 closed 4 years ago
Some areas where I'd like feedback:
Given that this is the middleware route, do we need any production-oriented changes to swarm-compose.yaml?
Given that this is the middleware route, do we need any production-oriented changes to swarm-compose.yaml?
I'm less familiar with swarm and how it handles env vars, but for production, I think only the env vars for telling boltwall how to connect with your node are required.
@bucko13 We provide HOT_WALLET_PASS, HOT_WALLET_ADDRESS, LND_SOCKET, and the mounted lnd volume for connecting the API to LND. The first two are via docker secrets, so they aren't explicit parameters (only visible to the internal container process).
Boltwall needs a few more (you can see them in the updated docker-compose, also mentioned in the testing directions here). Let me know if/where I need to add them for swarm to work.
Can boltwall ingest the lnd files for auth purposes? I think we'd rather use the mounted volume for LND_MACAROON and LND_TLS_CERT, if that's possible. For CAVEAT_KEY and SESSION_SECRET, we can add these to docker secrets as long as we generate them during the init script.
It cannot at the moment but it's an update I plan to make. I think it would still be good to support raw credentials though as it allows a core operator to connect to an existing and/or remote node rather than have to spin one up as part of the same "package".
I realize that all of that is still needed anyway because of the anchoring requirements, but it seems like it might be worthwhile to aim for as much separation/flexibility as possible where we can. Even just with this setup it can allow an operator to receive payments to a node separate from their anchoring wallet. In most cases this may not be desirable since you have to manually top up the anchoring wallet, but I could see use cases for it, e.g. privacy or providing liquidity to other routing nodes you might be running).
The paths would probably still need to be passed in environment variables anyway though as that is how boltwall handles its ln-service initialization. I think that would be good anyway as in the api service it seems to currently be hardcoded while the socket connection is in an env var, whereas all three are needed for connection so seems like it would make sense to parameterize all of them in env vars/secrets.
It would be nice if this would optionally accept a winston logger object so that the output generated from the boltwall endpoints matches that of the rest of the application.
Yes I totally agree. Now that boltwall is more of an “official” project of ours, spending some time on a little cleanup I think is definitely something I’d like to do. I’ve been doing a bit of this in parallel with this PR as they’ve come up (some necessary to get things to work). I will add this to the list!
To that point actually, any other suggestions for the api that would be worthwhile upgrades, this would be a good opportunity to play around with boltwall and find out.
LSAT and logging will be two big ones. I’m also curious about the error messaging and how those are formatted and communicated.
There is one particular error that I saw that I feel should be caught and logged as information, not an error. This occurs when you attempt to retrieve an invoice when there is already a currently outstanding unpaid invoice.
There was a problem creating hodl invoice: [ 503, 'UnexpectedAddHodlInvoiceError', { err: Error: 2 UNKNOWN: invoice with payment hash already exists at Object.exports.createStatusError (/home/node/app/node_modules/ln-service/node_modules/grpc/src/common.js:91:15) at Object.onReceiveStatus (/home/node/app/node_modules/ln-service/node_modules/grpc/src/client_interceptors.js:1204:28) at InterceptingListener._callNext (/home/node/app/node_modules/ln-service/node_modules/grpc/src/client_interceptors.js:568:42) at InterceptingListener.onReceiveStatus (/home/node/app/node_modules/ln-service/node_modules/grpc/src/client_interceptors.js:618:8) at callback (/home/node/app/node_modules/ln-service/node_modules/grpc/src/client_interceptors.js:845:24) { code: 2, metadata: [Metadata], details: 'invoice with payment hash already exists' } } ]
I think that should be caught and output something like this (assuming the use of winston) ->
2019-11-08T17:02:50.903Z [info] : Invoice with payment hash already exists : 73566c3bfc76f751352805c3dee254254096279976a522398a7ddb34982e8f57
An error occurred with that specific boltwall operation, but the API remains unfazed... its not bringing down the service. We should just log info like that I think.
Pausing work on this until boltwall has been updated for LSAT support. Will patch once boltwall API has been upgraded.
Fixed problem with lsat-js being unable to handle simnet invoices so this should be possible to test against running simnet nodes as in the PR description and verifying/parsing in the lsat playground.
@jacohend and @jasonbukowski - with regards to code that is no longer necessary, I wanted to confirm/clarify that the following should be removed:
node-lnd-mon-service/*
(whole directory)lnd-mon
service in docker-compose.ymlDockerfile.lnd-mon
lnd-mon
service in swarm yamllnd-mon
in README'gcr.io/chainpoint-registry/$REPO_NAME/node-lnd-mon-service:$COMMIT_SHA'
in cloud-build.ymlparseEnv.js
Are there any other services or code that can be done away with?
This PR removes redis tracking of invoices related to hash submissions and instead uses boltwall to manage endpoint authorization.
New endpoints include:
GET /boltwall/node
to get information about the node that will be paid (useful for establishing better connection to facilitate payment)POST /boltwall/hodl
to create a new hodl invoice. No body is necessary as a middleware will check for or create (if none is found) apaymentHash
and attach it to the req.body such that boltwall can generate a new invoice from itGET /invoice
If an invoice has been generated in association with the request (determined by macaroons attached as cookies) this will get information about the invoice.More comments about the code and new functionality will be included inline in the PR.
This PR also includes:
TESTING DIRECTIONS
make simnet
to start up a local simnet networkadminMacaroon
,cert
, and socket info. The first two should be in a localcredentials.json
file generated by the script. The socket info should bealice:10001
.env
file(Note: if you want to test against a running testnet node you can replace the relevant connection information above. Please note, that this must be a different node than the one you will be paying from for testing)
docker-compose up -d
from the chainpoint-core directory.docker-compose stop api
and then restart it with the logs (useful for testing) withdocker-compose up --build api
Other tools helpful for testing I recommend testing with postman (so you can easily parse and add headers) and the LSAT Playground. The LSAT playground will allow you to drop in the challenges to generate an LSAT token from it when making your requests.
TESTING ENDPOINTS I find it easiest to use Postman for this, but whatever works for you is obviously fine
GET /localhost/boltwall/node
to see your node's informationPOST localhost/hash
without any payment processing (include a properhash
in the request body). You should get a 402 error.WWW-Authenticate
header. This will include an invoice.Authorization
header.localhost:5000
and log in to RTL (password should be `foobar)POST localhost/hash
again. Now it should go through and the RTL dashboard should show the invoice as confirmed (no more hanging)